[ovs-dev,v17,2/3] OVN Remote Port Mirroring: northd changes to sync NB and SB

Changes which syncs the NB port mirrors with SB port mirrors.
Also test added to check the NB and SB sync

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

 northd/en-northd.c       |   4 +
 northd/inc-proc-northd.c |   4 +
 northd/northd.c          | 172 +++++++++++++++++++++++++++++++++++++++
 northd/northd.h          |   2 +
 tests/ovn-northd.at      | 102 +++++++++++++++++++++++
 5 files changed, 284 insertions(+)

On 11/27/22 15:14, Abhiram R N wrote:
> Changes which syncs the NB port mirrors with SB port mirrors.
> Also test added to check the NB and SB sync
> 
> 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
> 
>   northd/en-northd.c       |   4 +
>   northd/inc-proc-northd.c |   4 +
>   northd/northd.c          | 172 +++++++++++++++++++++++++++++++++++++++
>   northd/northd.h          |   2 +
>   tests/ovn-northd.at      | 102 +++++++++++++++++++++++
>   5 files changed, 284 insertions(+)
> 
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 93891b0b7..66ecc6573 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -78,6 +78,8 @@ void en_northd_run(struct engine_node *node, void *data)
>           EN_OVSDB_GET(engine_get_input("NB_acl", node));
>       input_data.nbrec_static_mac_binding_table =
>           EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
> +    input_data.nbrec_mirror_table =
> +        EN_OVSDB_GET(engine_get_input("NB_mirror", node));
>   
>       input_data.sbrec_sb_global_table =
>           EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> @@ -109,6 +111,8 @@ void en_northd_run(struct engine_node *node, void *data)
>           EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
>       input_data.sbrec_static_mac_binding_table =
>           EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
> +    input_data.sbrec_mirror_table =
> +        EN_OVSDB_GET(engine_get_input("SB_mirror", node));
>   
>       northd_run(&input_data, data,
>                  eng_ctx->ovnnb_idl_txn,
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 73f230b2c..7b7b250f3 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -52,6 +52,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
>       NB_NODE(acl, "acl") \
>       NB_NODE(logical_router, "logical_router") \
>       NB_NODE(qos, "qos") \
> +    NB_NODE(mirror, "mirror") \
>       NB_NODE(meter, "meter") \
>       NB_NODE(meter_band, "meter_band") \
>       NB_NODE(logical_router_port, "logical_router_port") \
> @@ -94,6 +95,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
>       SB_NODE(logical_flow, "logical_flow") \
>       SB_NODE(logical_dp_group, "logical_DP_group") \
>       SB_NODE(multicast_group, "multicast_group") \
> +    SB_NODE(mirror, "mirror") \
>       SB_NODE(meter, "meter") \
>       SB_NODE(meter_band, "meter_band") \
>       SB_NODE(datapath_binding, "datapath_binding") \
> @@ -176,6 +178,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>       engine_add_input(&en_northd, &en_nb_acl, NULL);
>       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
>       engine_add_input(&en_northd, &en_nb_qos, NULL);
> +    engine_add_input(&en_northd, &en_nb_mirror, NULL);
>       engine_add_input(&en_northd, &en_nb_meter, NULL);
>       engine_add_input(&en_northd, &en_nb_meter_band, NULL);
>       engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
> @@ -197,6 +200,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>       engine_add_input(&en_northd, &en_sb_encap, NULL);
>       engine_add_input(&en_northd, &en_sb_port_group, NULL);
>       engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
> +    engine_add_input(&en_northd, &en_sb_mirror, NULL);
>       engine_add_input(&en_northd, &en_sb_meter, NULL);
>       engine_add_input(&en_northd, &en_sb_meter_band, NULL);
>       engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 040f46e1a..16739983c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3239,6 +3239,89 @@ ovn_port_update_sbrec_chassis(
>       free(requested_chassis_sb);
>   }
>   
> +static void
> +do_sb_mirror_addition(struct northd_input *input_data,
> +                      const struct ovn_port *op)
> +{
> +    for (size_t i = 0; i < op->nbsp->n_mirror_rules; i++) {
> +        const struct sbrec_mirror *sb_mirror;
> +        SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
> +                                     input_data->sbrec_mirror_table) {
> +            if (!strcmp(sb_mirror->name,
> +                        op->nbsp->mirror_rules[i]->name)) {
> +                /* Add the value to SB */
> +                sbrec_port_binding_update_mirror_rules_addvalue(op->sb,
> +                                                                sb_mirror);
> +            }
> +        }
> +    }
> +}
> +
> +static void
> +sbrec_port_binding_update_mirror_rules(struct northd_input *input_data,
> +                                       const struct ovn_port *op)
> +{
> +    size_t i = 0;
> +    if (op->sb->n_mirror_rules > op->nbsp->n_mirror_rules) {
> +        /* Needs deletion in SB */
> +        struct shash nb_mirror_rules = SHASH_INITIALIZER(&nb_mirror_rules);
> +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
> +            shash_add(&nb_mirror_rules,
> +                                 op->nbsp->mirror_rules[i]->name,
> +                                 op->nbsp->mirror_rules[i]);
> +        }
> +
> +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
> +            if (!shash_find(&nb_mirror_rules,
> +                           op->sb->mirror_rules[i]->name)) {
> +                    sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
> +                                                 op->sb->mirror_rules[i]);
> +            }
> +        }
> +
> +        struct shash_node *node, *next;
> +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
> +            shash_delete(&nb_mirror_rules, node);
> +        }
> +        shash_destroy(&nb_mirror_rules);
> +
> +    } else if (op->sb->n_mirror_rules < op->nbsp->n_mirror_rules) {
> +        /* Needs addition in SB */
> +        do_sb_mirror_addition(input_data, op);
> +    } else if (op->sb->n_mirror_rules == op->nbsp->n_mirror_rules) {
> +        /*
> +         * Check if its the same mirrors on both SB and NB DBs
> +         * If not update accordingly.
> +         */
> +        bool needs_sb_addition = false;
> +        struct shash nb_mirror_rules = SHASH_INITIALIZER(&nb_mirror_rules);
> +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
> +            shash_add(&nb_mirror_rules,
> +                                 op->nbsp->mirror_rules[i]->name,
> +                                 op->nbsp->mirror_rules[i]);
> +        }
> +
> +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
> +            if (!shash_find(&nb_mirror_rules,
> +                           op->sb->mirror_rules[i]->name)) {
> +                    sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
> +                                                 op->sb->mirror_rules[i]);
> +                    needs_sb_addition = true;
> +            }
> +        }
> +
> +        struct shash_node *node, *next;
> +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
> +            shash_delete(&nb_mirror_rules, node);
> +        }
> +        shash_destroy(&nb_mirror_rules);
> +
> +        if (needs_sb_addition) {
> +            do_sb_mirror_addition(input_data, op);
> +        }
> +    }
> +}

This function is overly complicated. You don't need to add and delete 
individual values from the SBDB. Instead, what you can do is determine 
if there are any differences between the NBDB and SBDB, and if the 
answer is "yes" for any reason, then just do:

sbrec_port_binding_set_mirror_rules(op->sb, op->nbsp->mirror_rules, 
op->nbsp->n_mirror_rules);

This will greatly simplify the logic and make it far less error-prone. 
It also will make it so that northd controls all the values in the SBDB.

> +
>   static void
>   ovn_port_update_sbrec(struct northd_input *input_data,
>                         struct ovsdb_idl_txn *ovnsb_txn,
> @@ -3598,6 +3681,15 @@ ovn_port_update_sbrec(struct northd_input *input_data,
>           }
>           sbrec_port_binding_set_external_ids(op->sb, &ids);
>           smap_destroy(&ids);
> +
> +        if (!op->nbsp->n_mirror_rules) {
> +            /* Nothing is set. Clear mirror_rules from pb. */
> +            sbrec_port_binding_set_mirror_rules(op->sb, NULL, 0);
> +        } else {
> +            /* Check if SB DB update needed */
> +            sbrec_port_binding_update_mirror_rules(input_data, op);
> +        }
> +
>       }
>       if (op->tunnel_key != op->sb->tunnel_key) {
>           sbrec_port_binding_set_tunnel_key(op->sb, op->tunnel_key);
> @@ -15161,6 +15253,85 @@ sync_meters(struct northd_input *input_data,
>       shash_destroy(&sb_meters);
>   }
>   
> +static bool
> +mirror_needs_update(const struct nbrec_mirror *nb_mirror,
> +                  const struct sbrec_mirror *sb_mirror)
> +{
> +
> +    if (nb_mirror->index != sb_mirror->index) {
> +        return true;
> +    } else if (strcmp(nb_mirror->sink, sb_mirror->sink)) {
> +        return true;
> +    } else if (strcmp(nb_mirror->type, sb_mirror->type)) {
> +        return true;
> +    } else if (strcmp(nb_mirror->filter, sb_mirror->filter)) {
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +static void
> +sync_mirrors_iterate_nb_mirror(struct ovsdb_idl_txn *ovnsb_txn,
> +                             const char *mirror_name,
> +                             const struct nbrec_mirror *nb_mirror,
> +                             struct shash *sb_mirrors,
> +                             struct sset *used_sb_mirrors)
> +{
> +    const struct sbrec_mirror *sb_mirror;
> +    bool new_sb_mirror = false;
> +
> +    sb_mirror = shash_find_data(sb_mirrors, mirror_name);
> +    if (!sb_mirror) {
> +        sb_mirror = sbrec_mirror_insert(ovnsb_txn);
> +        sbrec_mirror_set_name(sb_mirror, mirror_name);
> +        shash_add(sb_mirrors, sb_mirror->name, sb_mirror);
> +        new_sb_mirror = true;
> +    }
> +    sset_add(used_sb_mirrors, mirror_name);
> +
> +    if ((new_sb_mirror) || mirror_needs_update(nb_mirror, sb_mirror)) {
> +        sbrec_mirror_set_filter(sb_mirror,nb_mirror->filter);
> +        sbrec_mirror_set_index(sb_mirror, nb_mirror->index);
> +        sbrec_mirror_set_sink(sb_mirror, nb_mirror->sink);
> +        sbrec_mirror_set_type(sb_mirror, nb_mirror->type);
> +    }
> +}
> +
> +static void
> +sync_mirrors(struct northd_input *input_data,
> +            struct ovsdb_idl_txn *ovnsb_txn)
> +{
> +    struct shash sb_mirrors = SHASH_INITIALIZER(&sb_mirrors);
> +    struct sset used_sb_mirrors = SSET_INITIALIZER(&used_sb_mirrors);
> +
> +    const struct sbrec_mirror *sb_mirror;
> +    SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror, input_data->sbrec_mirror_table) {
> +        shash_add(&sb_mirrors, sb_mirror->name, sb_mirror);
> +    }
> +
> +    const struct nbrec_mirror *nb_mirror;
> +    NBREC_MIRROR_TABLE_FOR_EACH (nb_mirror, input_data->nbrec_mirror_table) {
> +        sync_mirrors_iterate_nb_mirror(ovnsb_txn, nb_mirror->name, nb_mirror,
> +                                     &sb_mirrors, &used_sb_mirrors);
> +    }
> +
> +    const char *used_mirror;
> +    const char *used_mirror_next;
> +    SSET_FOR_EACH_SAFE (used_mirror, used_mirror_next, &used_sb_mirrors) {
> +        shash_find_and_delete(&sb_mirrors, used_mirror);
> +        sset_delete(&used_sb_mirrors, SSET_NODE_FROM_NAME(used_mirror));
> +    }
> +    sset_destroy(&used_sb_mirrors);
> +
> +    struct shash_node *node, *next;
> +    SHASH_FOR_EACH_SAFE (node, next, &sb_mirrors) {
> +        sbrec_mirror_delete(node->data);
> +        shash_delete(&sb_mirrors, node);
> +    }
> +    shash_destroy(&sb_mirrors);
> +}

The "used_mirrors" sset is unnecessary. The current algorithm is:

* Gather all SB mirrors into sb_mirrors shash
* For each matching NB mirror:
    * Add mirror to sb_mirrors shash
    * Add mirror name to used_mirrors sset
* For each mirror name in used_mirrors:
    * delete mirror from sb_mirrors
    * delete mirror name from used_mirrors
* For each mirror in sb_mirrors:
    * delete mirror from SBDB

This can be simplified to:

* Gather all SB mirrors into sb_mirrors shash
* For each matching NB mirror:
    * delete mirror from sb_mirrors
* For each mirror in sb_mirrors:
    * delete mirror from SBDB

> +
>   /*
>    * struct 'dns_info' is used to sync the DNS records between OVN Northbound db
>    * and Southbound db.
> @@ -15794,6 +15965,7 @@ ovnnb_db_run(struct northd_input *input_data,
>       sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs);
>       sync_port_groups(input_data, ovnsb_txn, &data->port_groups);
>       sync_meters(input_data, ovnsb_txn, &data->meter_groups);
> +    sync_mirrors(input_data, ovnsb_txn);
>       sync_dns_entries(input_data, ovnsb_txn, &data->datapaths);
>       cleanup_stale_fdb_entries(input_data, &data->datapaths);
>       stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> diff --git a/northd/northd.h b/northd/northd.h
> index ea9bd5797..1670177ed 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -35,6 +35,7 @@ struct northd_input {
>       const struct nbrec_acl_table *nbrec_acl_table;
>       const struct nbrec_static_mac_binding_table
>           *nbrec_static_mac_binding_table;
> +    const struct nbrec_mirror_table *nbrec_mirror_table;
>   
>       /* Southbound table references */
>       const struct sbrec_sb_global_table *sbrec_sb_global_table;
> @@ -53,6 +54,7 @@ struct northd_input {
>       const struct sbrec_chassis_private_table *sbrec_chassis_private_table;
>       const struct sbrec_static_mac_binding_table
>           *sbrec_static_mac_binding_table;
> +    const struct sbrec_mirror_table *sbrec_mirror_table;
>   
>       /* Indexes */
>       struct ovsdb_idl_index *sbrec_chassis_by_name;
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 02c00c062..466d16916 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2319,6 +2319,108 @@ check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
>   AT_CLEANUP
>   ])
>   
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([Check NB-SB mirrors sync])
> +AT_KEYWORDS([mirrors])
> +ovn_start
> +
> +check ovn-nbctl --wait=sb mirror-add mirror1 erspan 0 both 10.10.10.2
> +
> +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> +"10.10.10.2"
> +])

All of these AT_CHECK calls can use check_column instead. For the above, 
you can use

check_column 10.10.10.2 Mirror sink name=mirror1

Since there is only one row in the Mirror table, you might even be able 
to leave off that last bit.

check_column 10.10.10.2 Mirror sink

But I'm not 100% sure that will work.

> +
> +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> +erspan
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> +0
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> +both
> +])
> +
> +check ovn-nbctl --wait=sb \
> +    -- set mirror . sink=192.168.1.13
> +
> +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> +"192.168.1.13"
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> +erspan
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> +0
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> +both
> +])
> +
> +check ovn-nbctl --wait=sb \
> +    -- set mirror . type=gre
> +
> +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> +gre
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> +"192.168.1.13"
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> +0
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> +both
> +])
> +
> +check ovn-nbctl --wait=sb \
> +    -- set mirror . index=12
> +
> +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> +12
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> +gre
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> +"192.168.1.13"
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> +both
> +])
> +
> +check ovn-nbctl --wait=sb \
> +    -- set mirror . filter=to-lport
> +
> +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> +to-lport
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> +12
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> +gre
> +])
> +
> +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> +"192.168.1.13"
> +])
> +
> +AT_CLEANUP
> +])

This test is not very exhaustive. It should also:
   * Ensure that *all* mirrors are in both the NBDB and SBDB
   * Ensure that if a mirror is deleted from NBDB it is also deleted 
from SBDB
   * Try attaching mirrors to lsps and ensure that mirror_rules match in 
the SBDB as well. In addition, try adding/deleting mirror rules in the 
LSP and ensure that the mirror rules are represented in the SB 
Port_Bindings.



> +
>   OVN_FOR_EACH_NORTHD_NO_HV([
>   AT_SETUP([ACL skip hints for stateless config])
>   AT_KEYWORDS([acl])

Hi Mark,

Thanks for your review.
Please see replies inline below.

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

> On 11/27/22 15:14, Abhiram R N wrote:
> > Changes which syncs the NB port mirrors with SB port mirrors.
> > Also test added to check the NB and SB sync
> >
> > 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
> >
> >   northd/en-northd.c       |   4 +
> >   northd/inc-proc-northd.c |   4 +
> >   northd/northd.c          | 172 +++++++++++++++++++++++++++++++++++++++
> >   northd/northd.h          |   2 +
> >   tests/ovn-northd.at      | 102 +++++++++++++++++++++++
> >   5 files changed, 284 insertions(+)
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 93891b0b7..66ecc6573 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -78,6 +78,8 @@ void en_northd_run(struct engine_node *node, void
> *data)
> >           EN_OVSDB_GET(engine_get_input("NB_acl", node));
> >       input_data.nbrec_static_mac_binding_table =
> >           EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
> > +    input_data.nbrec_mirror_table =
> > +        EN_OVSDB_GET(engine_get_input("NB_mirror", node));
> >
> >       input_data.sbrec_sb_global_table =
> >           EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> > @@ -109,6 +111,8 @@ void en_northd_run(struct engine_node *node, void
> *data)
> >           EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
> >       input_data.sbrec_static_mac_binding_table =
> >           EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
> > +    input_data.sbrec_mirror_table =
> > +        EN_OVSDB_GET(engine_get_input("SB_mirror", node));
> >
> >       northd_run(&input_data, data,
> >                  eng_ctx->ovnnb_idl_txn,
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index 73f230b2c..7b7b250f3 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -52,6 +52,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> >       NB_NODE(acl, "acl") \
> >       NB_NODE(logical_router, "logical_router") \
> >       NB_NODE(qos, "qos") \
> > +    NB_NODE(mirror, "mirror") \
> >       NB_NODE(meter, "meter") \
> >       NB_NODE(meter_band, "meter_band") \
> >       NB_NODE(logical_router_port, "logical_router_port") \
> > @@ -94,6 +95,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> >       SB_NODE(logical_flow, "logical_flow") \
> >       SB_NODE(logical_dp_group, "logical_DP_group") \
> >       SB_NODE(multicast_group, "multicast_group") \
> > +    SB_NODE(mirror, "mirror") \
> >       SB_NODE(meter, "meter") \
> >       SB_NODE(meter_band, "meter_band") \
> >       SB_NODE(datapath_binding, "datapath_binding") \
> > @@ -176,6 +178,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >       engine_add_input(&en_northd, &en_nb_acl, NULL);
> >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> >       engine_add_input(&en_northd, &en_nb_qos, NULL);
> > +    engine_add_input(&en_northd, &en_nb_mirror, NULL);
> >       engine_add_input(&en_northd, &en_nb_meter, NULL);
> >       engine_add_input(&en_northd, &en_nb_meter_band, NULL);
> >       engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
> > @@ -197,6 +200,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >       engine_add_input(&en_northd, &en_sb_encap, NULL);
> >       engine_add_input(&en_northd, &en_sb_port_group, NULL);
> >       engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
> > +    engine_add_input(&en_northd, &en_sb_mirror, NULL);
> >       engine_add_input(&en_northd, &en_sb_meter, NULL);
> >       engine_add_input(&en_northd, &en_sb_meter_band, NULL);
> >       engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 040f46e1a..16739983c 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3239,6 +3239,89 @@ ovn_port_update_sbrec_chassis(
> >       free(requested_chassis_sb);
> >   }
> >
> > +static void
> > +do_sb_mirror_addition(struct northd_input *input_data,
> > +                      const struct ovn_port *op)
> > +{
> > +    for (size_t i = 0; i < op->nbsp->n_mirror_rules; i++) {
> > +        const struct sbrec_mirror *sb_mirror;
> > +        SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
> > +                                     input_data->sbrec_mirror_table) {
> > +            if (!strcmp(sb_mirror->name,
> > +                        op->nbsp->mirror_rules[i]->name)) {
> > +                /* Add the value to SB */
> > +                sbrec_port_binding_update_mirror_rules_addvalue(op->sb,
> > +
> sb_mirror);
> > +            }
> > +        }
> > +    }
> > +}
> > +
> > +static void
> > +sbrec_port_binding_update_mirror_rules(struct northd_input *input_data,
> > +                                       const struct ovn_port *op)
> > +{
> > +    size_t i = 0;
> > +    if (op->sb->n_mirror_rules > op->nbsp->n_mirror_rules) {
> > +        /* Needs deletion in SB */
> > +        struct shash nb_mirror_rules =
> SHASH_INITIALIZER(&nb_mirror_rules);
> > +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
> > +            shash_add(&nb_mirror_rules,
> > +                                 op->nbsp->mirror_rules[i]->name,
> > +                                 op->nbsp->mirror_rules[i]);
> > +        }
> > +
> > +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
> > +            if (!shash_find(&nb_mirror_rules,
> > +                           op->sb->mirror_rules[i]->name)) {
> > +
> sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
> > +
>  op->sb->mirror_rules[i]);
> > +            }
> > +        }
> > +
> > +        struct shash_node *node, *next;
> > +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
> > +            shash_delete(&nb_mirror_rules, node);
> > +        }
> > +        shash_destroy(&nb_mirror_rules);
> > +
> > +    } else if (op->sb->n_mirror_rules < op->nbsp->n_mirror_rules) {
> > +        /* Needs addition in SB */
> > +        do_sb_mirror_addition(input_data, op);
> > +    } else if (op->sb->n_mirror_rules == op->nbsp->n_mirror_rules) {
> > +        /*
> > +         * Check if its the same mirrors on both SB and NB DBs
> > +         * If not update accordingly.
> > +         */
> > +        bool needs_sb_addition = false;
> > +        struct shash nb_mirror_rules =
> SHASH_INITIALIZER(&nb_mirror_rules);
> > +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
> > +            shash_add(&nb_mirror_rules,
> > +                                 op->nbsp->mirror_rules[i]->name,
> > +                                 op->nbsp->mirror_rules[i]);
> > +        }
> > +
> > +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
> > +            if (!shash_find(&nb_mirror_rules,
> > +                           op->sb->mirror_rules[i]->name)) {
> > +
> sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
> > +
>  op->sb->mirror_rules[i]);
> > +                    needs_sb_addition = true;
> > +            }
> > +        }
> > +
> > +        struct shash_node *node, *next;
> > +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
> > +            shash_delete(&nb_mirror_rules, node);
> > +        }
> > +        shash_destroy(&nb_mirror_rules);
> > +
> > +        if (needs_sb_addition) {
> > +            do_sb_mirror_addition(input_data, op);
> > +        }
> > +    }
> > +}
>
> This function is overly complicated. You don't need to add and delete
> individual values from the SBDB. Instead, what you can do is determine
> if there are any differences between the NBDB and SBDB, and if the
> answer is "yes" for any reason, then just do:
>
> sbrec_port_binding_set_mirror_rules(op->sb, op->nbsp->mirror_rules,
> op->nbsp->n_mirror_rules);
>
> This will greatly simplify the logic and make it far less error-prone.
> It also will make it so that northd controls all the values in the SBDB.
>
I am afraid we cannot use 'sbrec_port_binding_set_mirror_rules'.
It throws a warning as below.
northd/northd.c:3701:49: warning: incompatible pointer types passing
'struct nbrec_mirror **const' to parameter of type 'struct sbrec_mirror **'
[-Wincompatible-pointer-types]
                                                op->nbsp->mirror_rules,
                                                ^~~~~~~~~~~~~~~~~~~~~~
./lib/ovn-sb-idl.h:5772:99: note: passing argument to parameter
'mirror_rules' here
void sbrec_port_binding_set_mirror_rules(const struct sbrec_port_binding *,
struct sbrec_mirror **mirror_rules, size_t n_mirror_rules);

I tried setting it through some typecasting. But the tests did not pass as
well)
On a deeper look at the API, the Mirror rules to be set in the Port Binding
table of
SB DB are the uuids of the Mirrors in the SB DB. (And cannot be copied over
from NB directly)
If it were to be a character array/string this works. But it doesn't work
for us.
Hope it makes sense?   [Incase I have mis-understood please clarify.]

Regarding the logic,
it just handles 3 cases, for a specific port binding
SB rules > NB rules,
NB rules > SB rules,
SB rules == NB rules


>
> > +
> >   static void
> >   ovn_port_update_sbrec(struct northd_input *input_data,
> >                         struct ovsdb_idl_txn *ovnsb_txn,
> > @@ -3598,6 +3681,15 @@ ovn_port_update_sbrec(struct northd_input
> *input_data,
> >           }
> >           sbrec_port_binding_set_external_ids(op->sb, &ids);
> >           smap_destroy(&ids);
> > +
> > +        if (!op->nbsp->n_mirror_rules) {
> > +            /* Nothing is set. Clear mirror_rules from pb. */
> > +            sbrec_port_binding_set_mirror_rules(op->sb, NULL, 0);
> > +        } else {
> > +            /* Check if SB DB update needed */
> > +            sbrec_port_binding_update_mirror_rules(input_data, op);
> > +        }
> > +
> >       }
> >       if (op->tunnel_key != op->sb->tunnel_key) {
> >           sbrec_port_binding_set_tunnel_key(op->sb, op->tunnel_key);
> > @@ -15161,6 +15253,85 @@ sync_meters(struct northd_input *input_data,
> >       shash_destroy(&sb_meters);
> >   }
> >
> > +static bool
> > +mirror_needs_update(const struct nbrec_mirror *nb_mirror,
> > +                  const struct sbrec_mirror *sb_mirror)
> > +{
> > +
> > +    if (nb_mirror->index != sb_mirror->index) {
> > +        return true;
> > +    } else if (strcmp(nb_mirror->sink, sb_mirror->sink)) {
> > +        return true;
> > +    } else if (strcmp(nb_mirror->type, sb_mirror->type)) {
> > +        return true;
> > +    } else if (strcmp(nb_mirror->filter, sb_mirror->filter)) {
> > +        return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> > +static void
> > +sync_mirrors_iterate_nb_mirror(struct ovsdb_idl_txn *ovnsb_txn,
> > +                             const char *mirror_name,
> > +                             const struct nbrec_mirror *nb_mirror,
> > +                             struct shash *sb_mirrors,
> > +                             struct sset *used_sb_mirrors)
> > +{
> > +    const struct sbrec_mirror *sb_mirror;
> > +    bool new_sb_mirror = false;
> > +
> > +    sb_mirror = shash_find_data(sb_mirrors, mirror_name);
> > +    if (!sb_mirror) {
> > +        sb_mirror = sbrec_mirror_insert(ovnsb_txn);
> > +        sbrec_mirror_set_name(sb_mirror, mirror_name);
> > +        shash_add(sb_mirrors, sb_mirror->name, sb_mirror);
> > +        new_sb_mirror = true;
> > +    }
> > +    sset_add(used_sb_mirrors, mirror_name);
> > +
> > +    if ((new_sb_mirror) || mirror_needs_update(nb_mirror, sb_mirror)) {
> > +        sbrec_mirror_set_filter(sb_mirror,nb_mirror->filter);
> > +        sbrec_mirror_set_index(sb_mirror, nb_mirror->index);
> > +        sbrec_mirror_set_sink(sb_mirror, nb_mirror->sink);
> > +        sbrec_mirror_set_type(sb_mirror, nb_mirror->type);
> > +    }
> > +}
> > +
> > +static void
> > +sync_mirrors(struct northd_input *input_data,
> > +            struct ovsdb_idl_txn *ovnsb_txn)
> > +{
> > +    struct shash sb_mirrors = SHASH_INITIALIZER(&sb_mirrors);
> > +    struct sset used_sb_mirrors = SSET_INITIALIZER(&used_sb_mirrors);
> > +
> > +    const struct sbrec_mirror *sb_mirror;
> > +    SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
> input_data->sbrec_mirror_table) {
> > +        shash_add(&sb_mirrors, sb_mirror->name, sb_mirror);
> > +    }
> > +
> > +    const struct nbrec_mirror *nb_mirror;
> > +    NBREC_MIRROR_TABLE_FOR_EACH (nb_mirror,
> input_data->nbrec_mirror_table) {
> > +        sync_mirrors_iterate_nb_mirror(ovnsb_txn, nb_mirror->name,
> nb_mirror,
> > +                                     &sb_mirrors, &used_sb_mirrors);
> > +    }
> > +
> > +    const char *used_mirror;
> > +    const char *used_mirror_next;
> > +    SSET_FOR_EACH_SAFE (used_mirror, used_mirror_next,
> &used_sb_mirrors) {
> > +        shash_find_and_delete(&sb_mirrors, used_mirror);
> > +        sset_delete(&used_sb_mirrors, SSET_NODE_FROM_NAME(used_mirror));
> > +    }
> > +    sset_destroy(&used_sb_mirrors);
> > +
> > +    struct shash_node *node, *next;
> > +    SHASH_FOR_EACH_SAFE (node, next, &sb_mirrors) {
> > +        sbrec_mirror_delete(node->data);
> > +        shash_delete(&sb_mirrors, node);
> > +    }
> > +    shash_destroy(&sb_mirrors);
> > +}
>
> The "used_mirrors" sset is unnecessary. The current algorithm is:
>
> * Gather all SB mirrors into sb_mirrors shash
> * For each matching NB mirror:
>     * Add mirror to sb_mirrors shash
>     * Add mirror name to used_mirrors sset
> * For each mirror name in used_mirrors:
>     * delete mirror from sb_mirrors
>     * delete mirror name from used_mirrors
> * For each mirror in sb_mirrors:
>     * delete mirror from SBDB
>
> This can be simplified to:
>
> * Gather all SB mirrors into sb_mirrors shash
> * For each matching NB mirror:
>     * delete mirror from sb_mirrors
> * For each mirror in sb_mirrors:
>     * delete mirror from SBDB
>
> Agreed. Will change this.

> > +
> >   /*
> >    * struct 'dns_info' is used to sync the DNS records between OVN
> Northbound db
> >    * and Southbound db.
> > @@ -15794,6 +15965,7 @@ ovnnb_db_run(struct northd_input *input_data,
> >       sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs);
> >       sync_port_groups(input_data, ovnsb_txn, &data->port_groups);
> >       sync_meters(input_data, ovnsb_txn, &data->meter_groups);
> > +    sync_mirrors(input_data, ovnsb_txn);
> >       sync_dns_entries(input_data, ovnsb_txn, &data->datapaths);
> >       cleanup_stale_fdb_entries(input_data, &data->datapaths);
> >       stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> > diff --git a/northd/northd.h b/northd/northd.h
> > index ea9bd5797..1670177ed 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -35,6 +35,7 @@ struct northd_input {
> >       const struct nbrec_acl_table *nbrec_acl_table;
> >       const struct nbrec_static_mac_binding_table
> >           *nbrec_static_mac_binding_table;
> > +    const struct nbrec_mirror_table *nbrec_mirror_table;
> >
> >       /* Southbound table references */
> >       const struct sbrec_sb_global_table *sbrec_sb_global_table;
> > @@ -53,6 +54,7 @@ struct northd_input {
> >       const struct sbrec_chassis_private_table
> *sbrec_chassis_private_table;
> >       const struct sbrec_static_mac_binding_table
> >           *sbrec_static_mac_binding_table;
> > +    const struct sbrec_mirror_table *sbrec_mirror_table;
> >
> >       /* Indexes */
> >       struct ovsdb_idl_index *sbrec_chassis_by_name;
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 02c00c062..466d16916 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -2319,6 +2319,108 @@ check_meter_by_name NOT meter_me__${acl1}
> meter_me__${acl2}
> >   AT_CLEANUP
> >   ])
> >
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Check NB-SB mirrors sync])
> > +AT_KEYWORDS([mirrors])
> > +ovn_start
> > +
> > +check ovn-nbctl --wait=sb mirror-add mirror1 erspan 0 both 10.10.10.2
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> > +"10.10.10.2"
> > +])
>
> All of these AT_CHECK calls can use check_column instead. For the above,
> you can use
>
> check_column 10.10.10.2 Mirror sink name=mirror1
>
> Since there is only one row in the Mirror table, you might even be able
> to leave off that last bit.
>
> check_column 10.10.10.2 Mirror sink
>
> But I'm not 100% sure that will work.
>
> Sure, I will try out and change accordingly.

> > +
> > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> > +erspan
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> > +0
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> > +both
> > +])
> > +
> > +check ovn-nbctl --wait=sb \
> > +    -- set mirror . sink=192.168.1.13
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> > +"192.168.1.13"
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> > +erspan
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> > +0
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> > +both
> > +])
> > +
> > +check ovn-nbctl --wait=sb \
> > +    -- set mirror . type=gre
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> > +gre
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> > +"192.168.1.13"
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> > +0
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> > +both
> > +])
> > +
> > +check ovn-nbctl --wait=sb \
> > +    -- set mirror . index=12
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> > +12
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> > +gre
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> > +"192.168.1.13"
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> > +both
> > +])
> > +
> > +check ovn-nbctl --wait=sb \
> > +    -- set mirror . filter=to-lport
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> > +to-lport
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> > +12
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> > +gre
> > +])
> > +
> > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> > +"192.168.1.13"
> > +])
> > +
> > +AT_CLEANUP
> > +])
>
> This test is not very exhaustive. It should also:
>    * Ensure that *all* mirrors are in both the NBDB and SBDB
>    * Ensure that if a mirror is deleted from NBDB it is also deleted
> from SBDB
>    * Try attaching mirrors to lsps and ensure that mirror_rules match in
> the SBDB as well. In addition, try adding/deleting mirror rules in the
> LSP and ensure that the mirror rules are represented in the SB
> Port_Bindings.
>
> We had not made this exhaustive here because we are checking
extensively in ovn.at test cases (but ofcourse we verify OVSDB there).
Since OVS DB can be correct only if OVN SB DB is correct.
Pretty much add, delete and attach (Also bulk) is covered there.
If you insist I can extend here as well.
Let me know if you feel it's important to be kept here as well.


>
> > +
> >   OVN_FOR_EACH_NORTHD_NO_HV([
> >   AT_SETUP([ACL skip hints for stateless config])
> >   AT_KEYWORDS([acl])
>
> Thanks & Regards,
Abhiram R N

On 11/29/22 08:27, Abhiram R N wrote:
> Hi Mark,
> 
> Thanks for your review.
> Please see replies inline below.
> 
> On Tue, Nov 29, 2022 at 3:23 AM Mark Michelson <mmichels@redhat.com 
> <mailto:mmichels@redhat.com>> wrote:
> 
>     On 11/27/22 15:14, Abhiram R N wrote:
>      > Changes which syncs the NB port mirrors with SB port mirrors.
>      > Also test added to check the NB and SB sync
>      >
>      > Co-authored-by: Veda Barrenkala <vedabarrenkala@gmail.com
>     <mailto:vedabarrenkala@gmail.com>>
>      > Signed-off-by: Veda Barrenkala <vedabarrenkala@gmail.com
>     <mailto:vedabarrenkala@gmail.com>>
>      > Signed-off-by: Abhiram R N <abhiramrn@gmail.com
>     <mailto:abhiramrn@gmail.com>>
>      > ---
>      > v16-->v17: No changes
>      >
>      >   northd/en-northd.c       |   4 +
>      >   northd/inc-proc-northd.c |   4 +
>      >   northd/northd.c          | 172
>     +++++++++++++++++++++++++++++++++++++++
>      >   northd/northd.h          |   2 +
>      >   tests/ovn-northd.at <http://ovn-northd.at>      | 102
>     +++++++++++++++++++++++
>      >   5 files changed, 284 insertions(+)
>      >
>      > diff --git a/northd/en-northd.c b/northd/en-northd.c
>      > index 93891b0b7..66ecc6573 100644
>      > --- a/northd/en-northd.c
>      > +++ b/northd/en-northd.c
>      > @@ -78,6 +78,8 @@ void en_northd_run(struct engine_node *node,
>     void *data)
>      >           EN_OVSDB_GET(engine_get_input("NB_acl", node));
>      >       input_data.nbrec_static_mac_binding_table =
>      >           EN_OVSDB_GET(engine_get_input("NB_static_mac_binding",
>     node));
>      > +    input_data.nbrec_mirror_table =
>      > +        EN_OVSDB_GET(engine_get_input("NB_mirror", node));
>      >
>      >       input_data.sbrec_sb_global_table =
>      >           EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
>      > @@ -109,6 +111,8 @@ void en_northd_run(struct engine_node *node,
>     void *data)
>      >           EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
>      >       input_data.sbrec_static_mac_binding_table =
>      >           EN_OVSDB_GET(engine_get_input("SB_static_mac_binding",
>     node));
>      > +    input_data.sbrec_mirror_table =
>      > +        EN_OVSDB_GET(engine_get_input("SB_mirror", node));
>      >
>      >       northd_run(&input_data, data,
>      >                  eng_ctx->ovnnb_idl_txn,
>      > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
>      > index 73f230b2c..7b7b250f3 100644
>      > --- a/northd/inc-proc-northd.c
>      > +++ b/northd/inc-proc-northd.c
>      > @@ -52,6 +52,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
>      >       NB_NODE(acl, "acl") \
>      >       NB_NODE(logical_router, "logical_router") \
>      >       NB_NODE(qos, "qos") \
>      > +    NB_NODE(mirror, "mirror") \
>      >       NB_NODE(meter, "meter") \
>      >       NB_NODE(meter_band, "meter_band") \
>      >       NB_NODE(logical_router_port, "logical_router_port") \
>      > @@ -94,6 +95,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
>      >       SB_NODE(logical_flow, "logical_flow") \
>      >       SB_NODE(logical_dp_group, "logical_DP_group") \
>      >       SB_NODE(multicast_group, "multicast_group") \
>      > +    SB_NODE(mirror, "mirror") \
>      >       SB_NODE(meter, "meter") \
>      >       SB_NODE(meter_band, "meter_band") \
>      >       SB_NODE(datapath_binding, "datapath_binding") \
>      > @@ -176,6 +178,7 @@ void inc_proc_northd_init(struct
>     ovsdb_idl_loop *nb,
>      >       engine_add_input(&en_northd, &en_nb_acl, NULL);
>      >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
>      >       engine_add_input(&en_northd, &en_nb_qos, NULL);
>      > +    engine_add_input(&en_northd, &en_nb_mirror, NULL);
>      >       engine_add_input(&en_northd, &en_nb_meter, NULL);
>      >       engine_add_input(&en_northd, &en_nb_meter_band, NULL);
>      >       engine_add_input(&en_northd, &en_nb_logical_router_port, NULL);
>      > @@ -197,6 +200,7 @@ void inc_proc_northd_init(struct
>     ovsdb_idl_loop *nb,
>      >       engine_add_input(&en_northd, &en_sb_encap, NULL);
>      >       engine_add_input(&en_northd, &en_sb_port_group, NULL);
>      >       engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
>      > +    engine_add_input(&en_northd, &en_sb_mirror, NULL);
>      >       engine_add_input(&en_northd, &en_sb_meter, NULL);
>      >       engine_add_input(&en_northd, &en_sb_meter_band, NULL);
>      >       engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
>      > diff --git a/northd/northd.c b/northd/northd.c
>      > index 040f46e1a..16739983c 100644
>      > --- a/northd/northd.c
>      > +++ b/northd/northd.c
>      > @@ -3239,6 +3239,89 @@ ovn_port_update_sbrec_chassis(
>      >       free(requested_chassis_sb);
>      >   }
>      >
>      > +static void
>      > +do_sb_mirror_addition(struct northd_input *input_data,
>      > +                      const struct ovn_port *op)
>      > +{
>      > +    for (size_t i = 0; i < op->nbsp->n_mirror_rules; i++) {
>      > +        const struct sbrec_mirror *sb_mirror;
>      > +        SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
>      > +                                   
>       input_data->sbrec_mirror_table) {
>      > +            if (!strcmp(sb_mirror->name,
>      > +                        op->nbsp->mirror_rules[i]->name)) {
>      > +                /* Add the value to SB */
>      > +               
>     sbrec_port_binding_update_mirror_rules_addvalue(op->sb,
>      > +                                                               
>     sb_mirror);
>      > +            }
>      > +        }
>      > +    }
>      > +}
>      > +
>      > +static void
>      > +sbrec_port_binding_update_mirror_rules(struct northd_input
>     *input_data,
>      > +                                       const struct ovn_port *op)
>      > +{
>      > +    size_t i = 0;
>      > +    if (op->sb->n_mirror_rules > op->nbsp->n_mirror_rules) {
>      > +        /* Needs deletion in SB */
>      > +        struct shash nb_mirror_rules =
>     SHASH_INITIALIZER(&nb_mirror_rules);
>      > +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
>      > +            shash_add(&nb_mirror_rules,
>      > +                                 op->nbsp->mirror_rules[i]->name,
>      > +                                 op->nbsp->mirror_rules[i]);
>      > +        }
>      > +
>      > +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
>      > +            if (!shash_find(&nb_mirror_rules,
>      > +                           op->sb->mirror_rules[i]->name)) {
>      > +                   
>     sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
>      > +                                               
>       op->sb->mirror_rules[i]);
>      > +            }
>      > +        }
>      > +
>      > +        struct shash_node *node, *next;
>      > +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
>      > +            shash_delete(&nb_mirror_rules, node);
>      > +        }
>      > +        shash_destroy(&nb_mirror_rules);
>      > +
>      > +    } else if (op->sb->n_mirror_rules < op->nbsp->n_mirror_rules) {
>      > +        /* Needs addition in SB */
>      > +        do_sb_mirror_addition(input_data, op);
>      > +    } else if (op->sb->n_mirror_rules == op->nbsp->n_mirror_rules) {
>      > +        /*
>      > +         * Check if its the same mirrors on both SB and NB DBs
>      > +         * If not update accordingly.
>      > +         */
>      > +        bool needs_sb_addition = false;
>      > +        struct shash nb_mirror_rules =
>     SHASH_INITIALIZER(&nb_mirror_rules);
>      > +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
>      > +            shash_add(&nb_mirror_rules,
>      > +                                 op->nbsp->mirror_rules[i]->name,
>      > +                                 op->nbsp->mirror_rules[i]);
>      > +        }
>      > +
>      > +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
>      > +            if (!shash_find(&nb_mirror_rules,
>      > +                           op->sb->mirror_rules[i]->name)) {
>      > +                   
>     sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
>      > +                                               
>       op->sb->mirror_rules[i]);
>      > +                    needs_sb_addition = true;
>      > +            }
>      > +        }
>      > +
>      > +        struct shash_node *node, *next;
>      > +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
>      > +            shash_delete(&nb_mirror_rules, node);
>      > +        }
>      > +        shash_destroy(&nb_mirror_rules);
>      > +
>      > +        if (needs_sb_addition) {
>      > +            do_sb_mirror_addition(input_data, op);
>      > +        }
>      > +    }
>      > +}
> 
>     This function is overly complicated. You don't need to add and delete
>     individual values from the SBDB. Instead, what you can do is determine
>     if there are any differences between the NBDB and SBDB, and if the
>     answer is "yes" for any reason, then just do:
> 
>     sbrec_port_binding_set_mirror_rules(op->sb, op->nbsp->mirror_rules,
>     op->nbsp->n_mirror_rules);
> 
>     This will greatly simplify the logic and make it far less error-prone.
>     It also will make it so that northd controls all the values in the SBDB.
> 
> I am afraid we cannot use 'sbrec_port_binding_set_mirror_rules'.
> It throws a warning as below.
> northd/northd.c:3701:49: warning: incompatible pointer types passing 
> 'struct nbrec_mirror **const' to parameter of type 'struct sbrec_mirror 
> **' [-Wincompatible-pointer-types]
>                                                  op->nbsp->mirror_rules,
>                                                  ^~~~~~~~~~~~~~~~~~~~~~
> ./lib/ovn-sb-idl.h:5772:99: note: passing argument to parameter 
> 'mirror_rules' here
> void sbrec_port_binding_set_mirror_rules(const struct sbrec_port_binding 
> *, struct sbrec_mirror **mirror_rules, size_t n_mirror_rules);
> 
> I tried setting it through some typecasting. But the tests did not pass 
> as well)
> On a deeper look at the API, the Mirror rules to be set in the Port 
> Binding table of
> SB DB are the uuids of the Mirrors in the SB DB. (And cannot be copied 
> over from NB directly)
> If it were to be a character array/string this works. But it doesn't 
> work for us.
> Hope it makes sense?   [Incase I have mis-understood please clarify.]
> 
> Regarding the logic,
> it just handles 3 cases, for a specific port binding
> SB rules > NB rules,
> NB rules > SB rules,
> SB rules == NB rules

OK, that is my mistake. I should have realized that you can't set a NB 
mirror in the SBDB :)

There are still issues with the approach, though, including bugs I had 
not pointed out in my first review.

In the case where SB rules > NB rules, the code makes the assumption 
that the only necessary operation is to delete rows from the SBDB. 
However, this doesn't take into account the fact that somebody may have 
made simultaneous updates to the SBDB to add a mirror rule to a port 
binding and to the NBDB to alter a mirror rule on the corresponding 
logical switch port. In this case, the rogue SBDB mirror rule will be 
deleted properly, AND the existing SBDB mirror rule that corresponds 
with the altered NB rule will get deleted rather than being updated with 
the new value from the NB mirror rule.

The same assumption exists in reverse in the NB rules > SB rules case. 
Imagine someone alters a SB Port_Binding's mirror rule, and 
simultaneously adds a new mirror rule to the corresponding NB 
Logical_Switch_Port. In this case, the code will add the new NB mirror 
rule, AND it will add a new mirror_rule to the SB port binding for the 
altered NB mirror rule rather than updating the corresponding SB rule.

The problem is that even if the number of mirror rules is mismatched 
between NB and SB, a side-by-side comparison of the mirror rules must be 
done each time to determine if there are further differences between the 
two.

Furthermore, in the end, what truly matters is what's put in the 
northbound database. Any changes made by someone other than northd 
should be overwritten. If there is a conflict, ovn-northd is the "truth" 
in this situation. So it means that 
sbrec_port_binding_set_mirror_rules() with mirrors based on the northd 
logical switch port is the best way to handle any and all differences 
between NB and SB.

One suggestion would be to create a new structure in northd.c:

struct ovn_northd_mirror {
     struct nbrec_mirror *nb;
     struct sbrec_mirror *sb;
};

While it might seem good to hash this based on the common name between 
the nb and sb mirors, it actually is going to be better to hash on 
nb->_uuid instead. Place these in a shash structure and put this shash 
in the northd_data.

Then follow an algorithm like so:

For each ovn_port p:

Step 1) If the number of mirror rules in p->nbsp->miror_rules and 
p->sb->mirror_rules differ, then proceed to step 3. Otherwise proceed to 
step 2.

Step 2) For each p->nbsp->mirror_rule, find the corresponding 
ovn_northd_mirror. It it doesn't exist, then continue to the next mirror 
rule. Otherwise, if none of the mirror rules in p->sb->mirror_rules 
match mirror->sb, then there is a difference, and we need to proceed to 
Step 3. If all of the rules match, then we can stop right now.

Step 3) For each p->nbsp->mirror_rules, find the corresponding 
ovn_northd_mirror. If it doesn't exist, then continue to the next mirror 
rule. Otherwise, add mirror->sb to a list.

Step 4) Call sbrec_port_binding_set_mirror_rules() using the list of sb 
mirrors created in step 3.

This is a consistent method for ensuring that values in the NBDB dictate 
how to populate the SBDB, and any rogue elements added or changed in the 
SBDB will be overwritten by NB config.

Hopefully all of this makes sense.
Mark Michelson

> 
> 
>      > +
>      >   static void
>      >   ovn_port_update_sbrec(struct northd_input *input_data,
>      >                         struct ovsdb_idl_txn *ovnsb_txn,
>      > @@ -3598,6 +3681,15 @@ ovn_port_update_sbrec(struct northd_input
>     *input_data,
>      >           }
>      >           sbrec_port_binding_set_external_ids(op->sb, &ids);
>      >           smap_destroy(&ids);
>      > +
>      > +        if (!op->nbsp->n_mirror_rules) {
>      > +            /* Nothing is set. Clear mirror_rules from pb. */
>      > +            sbrec_port_binding_set_mirror_rules(op->sb, NULL, 0);
>      > +        } else {
>      > +            /* Check if SB DB update needed */
>      > +            sbrec_port_binding_update_mirror_rules(input_data, op);
>      > +        }
>      > +
>      >       }
>      >       if (op->tunnel_key != op->sb->tunnel_key) {
>      >           sbrec_port_binding_set_tunnel_key(op->sb, op->tunnel_key);
>      > @@ -15161,6 +15253,85 @@ sync_meters(struct northd_input *input_data,
>      >       shash_destroy(&sb_meters);
>      >   }
>      >
>      > +static bool
>      > +mirror_needs_update(const struct nbrec_mirror *nb_mirror,
>      > +                  const struct sbrec_mirror *sb_mirror)
>      > +{
>      > +
>      > +    if (nb_mirror->index != sb_mirror->index) {
>      > +        return true;
>      > +    } else if (strcmp(nb_mirror->sink, sb_mirror->sink)) {
>      > +        return true;
>      > +    } else if (strcmp(nb_mirror->type, sb_mirror->type)) {
>      > +        return true;
>      > +    } else if (strcmp(nb_mirror->filter, sb_mirror->filter)) {
>      > +        return true;
>      > +    }
>      > +
>      > +    return false;
>      > +}
>      > +
>      > +static void
>      > +sync_mirrors_iterate_nb_mirror(struct ovsdb_idl_txn *ovnsb_txn,
>      > +                             const char *mirror_name,
>      > +                             const struct nbrec_mirror *nb_mirror,
>      > +                             struct shash *sb_mirrors,
>      > +                             struct sset *used_sb_mirrors)
>      > +{
>      > +    const struct sbrec_mirror *sb_mirror;
>      > +    bool new_sb_mirror = false;
>      > +
>      > +    sb_mirror = shash_find_data(sb_mirrors, mirror_name);
>      > +    if (!sb_mirror) {
>      > +        sb_mirror = sbrec_mirror_insert(ovnsb_txn);
>      > +        sbrec_mirror_set_name(sb_mirror, mirror_name);
>      > +        shash_add(sb_mirrors, sb_mirror->name, sb_mirror);
>      > +        new_sb_mirror = true;
>      > +    }
>      > +    sset_add(used_sb_mirrors, mirror_name);
>      > +
>      > +    if ((new_sb_mirror) || mirror_needs_update(nb_mirror,
>     sb_mirror)) {
>      > +        sbrec_mirror_set_filter(sb_mirror,nb_mirror->filter);
>      > +        sbrec_mirror_set_index(sb_mirror, nb_mirror->index);
>      > +        sbrec_mirror_set_sink(sb_mirror, nb_mirror->sink);
>      > +        sbrec_mirror_set_type(sb_mirror, nb_mirror->type);
>      > +    }
>      > +}
>      > +
>      > +static void
>      > +sync_mirrors(struct northd_input *input_data,
>      > +            struct ovsdb_idl_txn *ovnsb_txn)
>      > +{
>      > +    struct shash sb_mirrors = SHASH_INITIALIZER(&sb_mirrors);
>      > +    struct sset used_sb_mirrors =
>     SSET_INITIALIZER(&used_sb_mirrors);
>      > +
>      > +    const struct sbrec_mirror *sb_mirror;
>      > +    SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
>     input_data->sbrec_mirror_table) {
>      > +        shash_add(&sb_mirrors, sb_mirror->name, sb_mirror);
>      > +    }
>      > +
>      > +    const struct nbrec_mirror *nb_mirror;
>      > +    NBREC_MIRROR_TABLE_FOR_EACH (nb_mirror,
>     input_data->nbrec_mirror_table) {
>      > +        sync_mirrors_iterate_nb_mirror(ovnsb_txn,
>     nb_mirror->name, nb_mirror,
>      > +                                     &sb_mirrors, &used_sb_mirrors);
>      > +    }
>      > +
>      > +    const char *used_mirror;
>      > +    const char *used_mirror_next;
>      > +    SSET_FOR_EACH_SAFE (used_mirror, used_mirror_next,
>     &used_sb_mirrors) {
>      > +        shash_find_and_delete(&sb_mirrors, used_mirror);
>      > +        sset_delete(&used_sb_mirrors,
>     SSET_NODE_FROM_NAME(used_mirror));
>      > +    }
>      > +    sset_destroy(&used_sb_mirrors);
>      > +
>      > +    struct shash_node *node, *next;
>      > +    SHASH_FOR_EACH_SAFE (node, next, &sb_mirrors) {
>      > +        sbrec_mirror_delete(node->data);
>      > +        shash_delete(&sb_mirrors, node);
>      > +    }
>      > +    shash_destroy(&sb_mirrors);
>      > +}
> 
>     The "used_mirrors" sset is unnecessary. The current algorithm is:
> 
>     * Gather all SB mirrors into sb_mirrors shash
>     * For each matching NB mirror:
>          * Add mirror to sb_mirrors shash
>          * Add mirror name to used_mirrors sset
>     * For each mirror name in used_mirrors:
>          * delete mirror from sb_mirrors
>          * delete mirror name from used_mirrors
>     * For each mirror in sb_mirrors:
>          * delete mirror from SBDB
> 
>     This can be simplified to:
> 
>     * Gather all SB mirrors into sb_mirrors shash
>     * For each matching NB mirror:
>          * delete mirror from sb_mirrors
>     * For each mirror in sb_mirrors:
>          * delete mirror from SBDB
> 
> Agreed. Will change this.
> 
>      > +
>      >   /*
>      >    * struct 'dns_info' is used to sync the DNS records between
>     OVN Northbound db
>      >    * and Southbound db.
>      > @@ -15794,6 +15965,7 @@ ovnnb_db_run(struct northd_input *input_data,
>      >       sync_lbs(input_data, ovnsb_txn, &data->datapaths, &data->lbs);
>      >       sync_port_groups(input_data, ovnsb_txn, &data->port_groups);
>      >       sync_meters(input_data, ovnsb_txn, &data->meter_groups);
>      > +    sync_mirrors(input_data, ovnsb_txn);
>      >       sync_dns_entries(input_data, ovnsb_txn, &data->datapaths);
>      >       cleanup_stale_fdb_entries(input_data, &data->datapaths);
>      >       stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
>      > diff --git a/northd/northd.h b/northd/northd.h
>      > index ea9bd5797..1670177ed 100644
>      > --- a/northd/northd.h
>      > +++ b/northd/northd.h
>      > @@ -35,6 +35,7 @@ struct northd_input {
>      >       const struct nbrec_acl_table *nbrec_acl_table;
>      >       const struct nbrec_static_mac_binding_table
>      >           *nbrec_static_mac_binding_table;
>      > +    const struct nbrec_mirror_table *nbrec_mirror_table;
>      >
>      >       /* Southbound table references */
>      >       const struct sbrec_sb_global_table *sbrec_sb_global_table;
>      > @@ -53,6 +54,7 @@ struct northd_input {
>      >       const struct sbrec_chassis_private_table
>     *sbrec_chassis_private_table;
>      >       const struct sbrec_static_mac_binding_table
>      >           *sbrec_static_mac_binding_table;
>      > +    const struct sbrec_mirror_table *sbrec_mirror_table;
>      >
>      >       /* Indexes */
>      >       struct ovsdb_idl_index *sbrec_chassis_by_name;
>      > diff --git a/tests/ovn-northd.at <http://ovn-northd.at>
>     b/tests/ovn-northd.at <http://ovn-northd.at>
>      > index 02c00c062..466d16916 100644
>      > --- a/tests/ovn-northd.at <http://ovn-northd.at>
>      > +++ b/tests/ovn-northd.at <http://ovn-northd.at>
>      > @@ -2319,6 +2319,108 @@ check_meter_by_name NOT meter_me__${acl1}
>     meter_me__${acl2}
>      >   AT_CLEANUP
>      >   ])
>      >
>      > +OVN_FOR_EACH_NORTHD_NO_HV([
>      > +AT_SETUP([Check NB-SB mirrors sync])
>      > +AT_KEYWORDS([mirrors])
>      > +ovn_start
>      > +
>      > +check ovn-nbctl --wait=sb mirror-add mirror1 erspan 0 both
>     10.10.10.2
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
>      > +"10.10.10.2"
>      > +])
> 
>     All of these AT_CHECK calls can use check_column instead. For the
>     above,
>     you can use
> 
>     check_column 10.10.10.2 Mirror sink name=mirror1
> 
>     Since there is only one row in the Mirror table, you might even be able
>     to leave off that last bit.
> 
>     check_column 10.10.10.2 Mirror sink
> 
>     But I'm not 100% sure that will work.
> 
> Sure, I will try out and change accordingly.
> 
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
>      > +erspan
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
>      > +0
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
>      > +both
>      > +])
>      > +
>      > +check ovn-nbctl --wait=sb \
>      > +    -- set mirror . sink=192.168.1.13
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
>      > +"192.168.1.13"
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
>      > +erspan
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
>      > +0
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
>      > +both
>      > +])
>      > +
>      > +check ovn-nbctl --wait=sb \
>      > +    -- set mirror . type=gre
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
>      > +gre
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
>      > +"192.168.1.13"
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
>      > +0
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
>      > +both
>      > +])
>      > +
>      > +check ovn-nbctl --wait=sb \
>      > +    -- set mirror . index=12
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
>      > +12
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
>      > +gre
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
>      > +"192.168.1.13"
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
>      > +both
>      > +])
>      > +
>      > +check ovn-nbctl --wait=sb \
>      > +    -- set mirror . filter=to-lport
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
>      > +to-lport
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
>      > +12
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
>      > +gre
>      > +])
>      > +
>      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
>      > +"192.168.1.13"
>      > +])
>      > +
>      > +AT_CLEANUP
>      > +])
> 
>     This test is not very exhaustive. It should also:
>         * Ensure that *all* mirrors are in both the NBDB and SBDB
>         * Ensure that if a mirror is deleted from NBDB it is also deleted
>     from SBDB
>         * Try attaching mirrors to lsps and ensure that mirror_rules
>     match in
>     the SBDB as well. In addition, try adding/deleting mirror rules in the
>     LSP and ensure that the mirror rules are represented in the SB
>     Port_Bindings.
> 
> We had not made this exhaustive here because we are checking
> extensively in ovn.at <http://ovn.at> test cases (but ofcourse we verify 
> OVSDB there).
> Since OVS DB can be correct only if OVN SB DB is correct.
> Pretty much add, delete and attach (Also bulk) is covered there.
> If you insist I can extend here as well.
> Let me know if you feel it's important to be kept here as well.

Ah I see the logic here. On the one hand, I agree with you since the 
coverage exists, and the data is implicitly checked.

I think the utility of having explicit SB checks as part of this patch 
is that it can potentially help us to debug issues more quickly if they 
arise. Imagine that a later code change breaks port mirroring. If a test 
exists whose sole purpose is to ensure NB-SB database synchronisation, 
then the options are:

1) Both tests fail. Therefore the developer knows that they must have 
made a mistake somewhere in northd that is causing the mirroring code to 
not get synchronized with the SBDB. Furthermore, the test case will 
probably show clearly that the problem exists with a specific type of 
change to the NBDB. The developer can tweak just that part of the code 
and then try again.

2) Only the ovn.at test fails. The developer can then rule out the 
possibility of NB-SB synchronization being a problem and can focus 
solely on changes made in ovn-controller that appear to affect what gets 
set in OVS from the SBDB.

However, if only the test in ovn.at exists, the developer has to debug 
all stages between the NBDB and OVS in order to try to determine at what 
stage the test is failing.

> 
> 
> 
>      > +
>      >   OVN_FOR_EACH_NORTHD_NO_HV([
>      >   AT_SETUP([ACL skip hints for stateless config])
>      >   AT_KEYWORDS([acl])
> 
> Thanks & Regards,
> Abhiram R N

Hi Mark,


On Tue, Nov 29, 2022 at 10:00 PM Mark Michelson <mmichels@redhat.com> wrote:

> On 11/29/22 08:27, Abhiram R N wrote:
> > Hi Mark,
> >
> > Thanks for your review.
> > Please see replies inline below.
> >
> > On Tue, Nov 29, 2022 at 3:23 AM Mark Michelson <mmichels@redhat.com
> > <mailto:mmichels@redhat.com>> wrote:
> >
> >     On 11/27/22 15:14, Abhiram R N wrote:
> >      > Changes which syncs the NB port mirrors with SB port mirrors.
> >      > Also test added to check the NB and SB sync
> >      >
> >      > Co-authored-by: Veda Barrenkala <vedabarrenkala@gmail.com
> >     <mailto:vedabarrenkala@gmail.com>>
> >      > Signed-off-by: Veda Barrenkala <vedabarrenkala@gmail.com
> >     <mailto:vedabarrenkala@gmail.com>>
> >      > Signed-off-by: Abhiram R N <abhiramrn@gmail.com
> >     <mailto:abhiramrn@gmail.com>>
> >      > ---
> >      > v16-->v17: No changes
> >      >
> >      >   northd/en-northd.c       |   4 +
> >      >   northd/inc-proc-northd.c |   4 +
> >      >   northd/northd.c          | 172
> >     +++++++++++++++++++++++++++++++++++++++
> >      >   northd/northd.h          |   2 +
> >      >   tests/ovn-northd.at <http://ovn-northd.at>      | 102
> >     +++++++++++++++++++++++
> >      >   5 files changed, 284 insertions(+)
> >      >
> >      > diff --git a/northd/en-northd.c b/northd/en-northd.c
> >      > index 93891b0b7..66ecc6573 100644
> >      > --- a/northd/en-northd.c
> >      > +++ b/northd/en-northd.c
> >      > @@ -78,6 +78,8 @@ void en_northd_run(struct engine_node *node,
> >     void *data)
> >      >           EN_OVSDB_GET(engine_get_input("NB_acl", node));
> >      >       input_data.nbrec_static_mac_binding_table =
> >      >           EN_OVSDB_GET(engine_get_input("NB_static_mac_binding",
> >     node));
> >      > +    input_data.nbrec_mirror_table =
> >      > +        EN_OVSDB_GET(engine_get_input("NB_mirror", node));
> >      >
> >      >       input_data.sbrec_sb_global_table =
> >      >           EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
> >      > @@ -109,6 +111,8 @@ void en_northd_run(struct engine_node *node,
> >     void *data)
> >      >           EN_OVSDB_GET(engine_get_input("SB_chassis_private",
> node));
> >      >       input_data.sbrec_static_mac_binding_table =
> >      >           EN_OVSDB_GET(engine_get_input("SB_static_mac_binding",
> >     node));
> >      > +    input_data.sbrec_mirror_table =
> >      > +        EN_OVSDB_GET(engine_get_input("SB_mirror", node));
> >      >
> >      >       northd_run(&input_data, data,
> >      >                  eng_ctx->ovnnb_idl_txn,
> >      > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> >      > index 73f230b2c..7b7b250f3 100644
> >      > --- a/northd/inc-proc-northd.c
> >      > +++ b/northd/inc-proc-northd.c
> >      > @@ -52,6 +52,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> >      >       NB_NODE(acl, "acl") \
> >      >       NB_NODE(logical_router, "logical_router") \
> >      >       NB_NODE(qos, "qos") \
> >      > +    NB_NODE(mirror, "mirror") \
> >      >       NB_NODE(meter, "meter") \
> >      >       NB_NODE(meter_band, "meter_band") \
> >      >       NB_NODE(logical_router_port, "logical_router_port") \
> >      > @@ -94,6 +95,7 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
> >      >       SB_NODE(logical_flow, "logical_flow") \
> >      >       SB_NODE(logical_dp_group, "logical_DP_group") \
> >      >       SB_NODE(multicast_group, "multicast_group") \
> >      > +    SB_NODE(mirror, "mirror") \
> >      >       SB_NODE(meter, "meter") \
> >      >       SB_NODE(meter_band, "meter_band") \
> >      >       SB_NODE(datapath_binding, "datapath_binding") \
> >      > @@ -176,6 +178,7 @@ void inc_proc_northd_init(struct
> >     ovsdb_idl_loop *nb,
> >      >       engine_add_input(&en_northd, &en_nb_acl, NULL);
> >      >       engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> >      >       engine_add_input(&en_northd, &en_nb_qos, NULL);
> >      > +    engine_add_input(&en_northd, &en_nb_mirror, NULL);
> >      >       engine_add_input(&en_northd, &en_nb_meter, NULL);
> >      >       engine_add_input(&en_northd, &en_nb_meter_band, NULL);
> >      >       engine_add_input(&en_northd, &en_nb_logical_router_port,
> NULL);
> >      > @@ -197,6 +200,7 @@ void inc_proc_northd_init(struct
> >     ovsdb_idl_loop *nb,
> >      >       engine_add_input(&en_northd, &en_sb_encap, NULL);
> >      >       engine_add_input(&en_northd, &en_sb_port_group, NULL);
> >      >       engine_add_input(&en_northd, &en_sb_logical_dp_group, NULL);
> >      > +    engine_add_input(&en_northd, &en_sb_mirror, NULL);
> >      >       engine_add_input(&en_northd, &en_sb_meter, NULL);
> >      >       engine_add_input(&en_northd, &en_sb_meter_band, NULL);
> >      >       engine_add_input(&en_northd, &en_sb_datapath_binding, NULL);
> >      > diff --git a/northd/northd.c b/northd/northd.c
> >      > index 040f46e1a..16739983c 100644
> >      > --- a/northd/northd.c
> >      > +++ b/northd/northd.c
> >      > @@ -3239,6 +3239,89 @@ ovn_port_update_sbrec_chassis(
> >      >       free(requested_chassis_sb);
> >      >   }
> >      >
> >      > +static void
> >      > +do_sb_mirror_addition(struct northd_input *input_data,
> >      > +                      const struct ovn_port *op)
> >      > +{
> >      > +    for (size_t i = 0; i < op->nbsp->n_mirror_rules; i++) {
> >      > +        const struct sbrec_mirror *sb_mirror;
> >      > +        SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
> >      > +
> >       input_data->sbrec_mirror_table) {
> >      > +            if (!strcmp(sb_mirror->name,
> >      > +                        op->nbsp->mirror_rules[i]->name)) {
> >      > +                /* Add the value to SB */
> >      > +
> >     sbrec_port_binding_update_mirror_rules_addvalue(op->sb,
> >      > +
> >     sb_mirror);
> >      > +            }
> >      > +        }
> >      > +    }
> >      > +}
> >      > +
> >      > +static void
> >      > +sbrec_port_binding_update_mirror_rules(struct northd_input
> >     *input_data,
> >      > +                                       const struct ovn_port *op)
> >      > +{
> >      > +    size_t i = 0;
> >      > +    if (op->sb->n_mirror_rules > op->nbsp->n_mirror_rules) {
> >      > +        /* Needs deletion in SB */
> >      > +        struct shash nb_mirror_rules =
> >     SHASH_INITIALIZER(&nb_mirror_rules);
> >      > +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
> >      > +            shash_add(&nb_mirror_rules,
> >      > +                                 op->nbsp->mirror_rules[i]->name,
> >      > +                                 op->nbsp->mirror_rules[i]);
> >      > +        }
> >      > +
> >      > +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
> >      > +            if (!shash_find(&nb_mirror_rules,
> >      > +                           op->sb->mirror_rules[i]->name)) {
> >      > +
> >     sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
> >      > +
> >       op->sb->mirror_rules[i]);
> >      > +            }
> >      > +        }
> >      > +
> >      > +        struct shash_node *node, *next;
> >      > +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
> >      > +            shash_delete(&nb_mirror_rules, node);
> >      > +        }
> >      > +        shash_destroy(&nb_mirror_rules);
> >      > +
> >      > +    } else if (op->sb->n_mirror_rules <
> op->nbsp->n_mirror_rules) {
> >      > +        /* Needs addition in SB */
> >      > +        do_sb_mirror_addition(input_data, op);
> >      > +    } else if (op->sb->n_mirror_rules ==
> op->nbsp->n_mirror_rules) {
> >      > +        /*
> >      > +         * Check if its the same mirrors on both SB and NB DBs
> >      > +         * If not update accordingly.
> >      > +         */
> >      > +        bool needs_sb_addition = false;
> >      > +        struct shash nb_mirror_rules =
> >     SHASH_INITIALIZER(&nb_mirror_rules);
> >      > +        for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
> >      > +            shash_add(&nb_mirror_rules,
> >      > +                                 op->nbsp->mirror_rules[i]->name,
> >      > +                                 op->nbsp->mirror_rules[i]);
> >      > +        }
> >      > +
> >      > +        for (i = 0; i < op->sb->n_mirror_rules; i++) {
> >      > +            if (!shash_find(&nb_mirror_rules,
> >      > +                           op->sb->mirror_rules[i]->name)) {
> >      > +
> >     sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
> >      > +
> >       op->sb->mirror_rules[i]);
> >      > +                    needs_sb_addition = true;
> >      > +            }
> >      > +        }
> >      > +
> >      > +        struct shash_node *node, *next;
> >      > +        SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
> >      > +            shash_delete(&nb_mirror_rules, node);
> >      > +        }
> >      > +        shash_destroy(&nb_mirror_rules);
> >      > +
> >      > +        if (needs_sb_addition) {
> >      > +            do_sb_mirror_addition(input_data, op);
> >      > +        }
> >      > +    }
> >      > +}
> >
> >     This function is overly complicated. You don't need to add and delete
> >     individual values from the SBDB. Instead, what you can do is
> determine
> >     if there are any differences between the NBDB and SBDB, and if the
> >     answer is "yes" for any reason, then just do:
> >
> >     sbrec_port_binding_set_mirror_rules(op->sb, op->nbsp->mirror_rules,
> >     op->nbsp->n_mirror_rules);
> >
> >     This will greatly simplify the logic and make it far less
> error-prone.
> >     It also will make it so that northd controls all the values in the
> SBDB.
> >
> > I am afraid we cannot use 'sbrec_port_binding_set_mirror_rules'.
> > It throws a warning as below.
> > northd/northd.c:3701:49: warning: incompatible pointer types passing
> > 'struct nbrec_mirror **const' to parameter of type 'struct sbrec_mirror
> > **' [-Wincompatible-pointer-types]
> >                                                  op->nbsp->mirror_rules,
> >                                                  ^~~~~~~~~~~~~~~~~~~~~~
> > ./lib/ovn-sb-idl.h:5772:99: note: passing argument to parameter
> > 'mirror_rules' here
> > void sbrec_port_binding_set_mirror_rules(const struct sbrec_port_binding
> > *, struct sbrec_mirror **mirror_rules, size_t n_mirror_rules);
> >
> > I tried setting it through some typecasting. But the tests did not pass
> > as well)
> > On a deeper look at the API, the Mirror rules to be set in the Port
> > Binding table of
> > SB DB are the uuids of the Mirrors in the SB DB. (And cannot be copied
> > over from NB directly)
> > If it were to be a character array/string this works. But it doesn't
> > work for us.
> > Hope it makes sense?   [Incase I have mis-understood please clarify.]
> >
> > Regarding the logic,
> > it just handles 3 cases, for a specific port binding
> > SB rules > NB rules,
> > NB rules > SB rules,
> > SB rules == NB rules
>
> OK, that is my mistake. I should have realized that you can't set a NB
> mirror in the SBDB :)
>
> There are still issues with the approach, though, including bugs I had
> not pointed out in my first review.
>
> In the case where SB rules > NB rules, the code makes the assumption
> that the only necessary operation is to delete rows from the SBDB.
> However, this doesn't take into account the fact that somebody may have
> made simultaneous updates to the SBDB to add a mirror rule to a port
> binding and to the NBDB to alter a mirror rule on the corresponding
> logical switch port. In this case, the rogue SBDB mirror rule will be
> deleted properly, AND the existing SBDB mirror rule that corresponds
> with the altered NB rule will get deleted rather than being updated with
> the new value from the NB mirror rule.
>
> The same assumption exists in reverse in the NB rules > SB rules case.
> Imagine someone alters a SB Port_Binding's mirror rule, and
> simultaneously adds a new mirror rule to the corresponding NB
> Logical_Switch_Port. In this case, the code will add the new NB mirror
> rule, AND it will add a new mirror_rule to the SB port binding for the
> altered NB mirror rule rather than updating the corresponding SB rule.
>
> Got it. Understand the issue.
Basically when there is an addition there could be possible deletions also
in SB
So I need to check for both (Mostly similar to what I am doing in the ==
case) .

> The problem is that even if the number of mirror rules is mismatched
> between NB and SB, a side-by-side comparison of the mirror rules must be
> done each time to determine if there are further differences between the
> two.
>
> Furthermore, in the end, what truly matters is what's put in the
> northbound database. Any changes made by someone other than northd
> should be overwritten. If there is a conflict, ovn-northd is the "truth"
> in this situation. So it means that
> sbrec_port_binding_set_mirror_rules() with mirrors based on the northd
> logical switch port is the best way to handle any and all differences
> between NB and SB.
>
> One suggestion would be to create a new structure in northd.c:
>
> struct ovn_northd_mirror {
>      struct nbrec_mirror *nb;
>      struct sbrec_mirror *sb;
> };
>
> While it might seem good to hash this based on the common name between
> the nb and sb mirors, it actually is going to be better to hash on
> nb->_uuid instead. Place these in a shash structure and put this shash
> in the northd_data.
>
> Then follow an algorithm like so:
>
> For each ovn_port p:
>
> Step 1) If the number of mirror rules in p->nbsp->miror_rules and
> p->sb->mirror_rules differ, then proceed to step 3. Otherwise proceed to
> step 2.
>
> Step 2) For each p->nbsp->mirror_rule, find the corresponding
> ovn_northd_mirror. It it doesn't exist, then continue to the next mirror
> rule. Otherwise, if none of the mirror rules in p->sb->mirror_rules
> match mirror->sb, then there is a difference, and we need to proceed to
> Step 3. If all of the rules match, then we can stop right now.
>
> Step 3) For each p->nbsp->mirror_rules, find the corresponding
> ovn_northd_mirror. If it doesn't exist, then continue to the next mirror
> rule. Otherwise, add mirror->sb to a list.
>
> Step 4) Call sbrec_port_binding_set_mirror_rules() using the list of sb
> mirrors created in step 3.
>
> This is a consistent method for ensuring that values in the NBDB dictate
> how to populate the SBDB, and any rogue elements added or changed in the
> SBDB will be overwritten by NB config.
>
> Hopefully all of this makes sense.
> Mark Michelson
>
Thanks for the detailed explanation.
Now that I understood the problem you explained,
I just thought that to keep it less complicated I can just do
what I am doing currently in the == case. (or just merge the > and < cases)
for when we get call to 'sbrec_port_binding_update_mirror_rules'
irrespective of whether number of mirror rules match or not in NB and SB
i.e  check and do based on nb mirror rules
          any deletions needed in SB
          AND
          any additions needed in SB
This should solve the problems which you explained above.
Hope this is okay!.. (I pasted the code for your reference)

static void
check_and_do_sb_mirror_deletion(const struct ovn_port *op)
{
    size_t i = 0;
    struct shash nb_mirror_rules = SHASH_INITIALIZER(&nb_mirror_rules);

    for (i = 0; i < op->nbsp->n_mirror_rules; i++) {
        shash_add(&nb_mirror_rules,
                  op->nbsp->mirror_rules[i]->name,
                  op->nbsp->mirror_rules[i]);
    }

    for (i = 0; i < op->sb->n_mirror_rules; i++) {
        if (!shash_find(&nb_mirror_rules,
                        op->sb->mirror_rules[i]->name)) {
            /* Delete from SB since its not present in NB*/
            sbrec_port_binding_update_mirror_rules_delvalue(op->sb,
                                             op->sb->mirror_rules[i]);
        }
    }

    struct shash_node *node, *next;
    SHASH_FOR_EACH_SAFE (node, next, &nb_mirror_rules) {
        shash_delete(&nb_mirror_rules, node);
    }
    shash_destroy(&nb_mirror_rules);
}

static void
check_and_do_sb_mirror_addition(struct northd_input *input_data,
                                const struct ovn_port *op)
{
    for (size_t i = 0; i < op->nbsp->n_mirror_rules; i++) {
        const struct sbrec_mirror *sb_mirror;
        SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
                                     input_data->sbrec_mirror_table) {
            if (!strcmp(sb_mirror->name,
                        op->nbsp->mirror_rules[i]->name)) {
                /* Add the value to SB */
                sbrec_port_binding_update_mirror_rules_addvalue(op->sb,
                                                                sb_mirror);
            }
        }
    }
}

static void
sbrec_port_binding_update_mirror_rules(struct northd_input *input_data,
                                       const struct ovn_port *op)
{
    check_and_do_sb_mirror_deletion(op);
    check_and_do_sb_mirror_addition(input_data, op);
}


> >
> >
> >      > +
> >      >   static void
> >      >   ovn_port_update_sbrec(struct northd_input *input_data,
> >      >                         struct ovsdb_idl_txn *ovnsb_txn,
> >      > @@ -3598,6 +3681,15 @@ ovn_port_update_sbrec(struct northd_input
> >     *input_data,
> >      >           }
> >      >           sbrec_port_binding_set_external_ids(op->sb, &ids);
> >      >           smap_destroy(&ids);
> >      > +
> >      > +        if (!op->nbsp->n_mirror_rules) {
> >      > +            /* Nothing is set. Clear mirror_rules from pb. */
> >      > +            sbrec_port_binding_set_mirror_rules(op->sb, NULL, 0);
> >      > +        } else {
> >      > +            /* Check if SB DB update needed */
> >      > +            sbrec_port_binding_update_mirror_rules(input_data,
> op);
> >      > +        }
> >      > +
> >      >       }
> >      >       if (op->tunnel_key != op->sb->tunnel_key) {
> >      >           sbrec_port_binding_set_tunnel_key(op->sb,
> op->tunnel_key);
> >      > @@ -15161,6 +15253,85 @@ sync_meters(struct northd_input
> *input_data,
> >      >       shash_destroy(&sb_meters);
> >      >   }
> >      >
> >      > +static bool
> >      > +mirror_needs_update(const struct nbrec_mirror *nb_mirror,
> >      > +                  const struct sbrec_mirror *sb_mirror)
> >      > +{
> >      > +
> >      > +    if (nb_mirror->index != sb_mirror->index) {
> >      > +        return true;
> >      > +    } else if (strcmp(nb_mirror->sink, sb_mirror->sink)) {
> >      > +        return true;
> >      > +    } else if (strcmp(nb_mirror->type, sb_mirror->type)) {
> >      > +        return true;
> >      > +    } else if (strcmp(nb_mirror->filter, sb_mirror->filter)) {
> >      > +        return true;
> >      > +    }
> >      > +
> >      > +    return false;
> >      > +}
> >      > +
> >      > +static void
> >      > +sync_mirrors_iterate_nb_mirror(struct ovsdb_idl_txn *ovnsb_txn,
> >      > +                             const char *mirror_name,
> >      > +                             const struct nbrec_mirror
> *nb_mirror,
> >      > +                             struct shash *sb_mirrors,
> >      > +                             struct sset *used_sb_mirrors)
> >      > +{
> >      > +    const struct sbrec_mirror *sb_mirror;
> >      > +    bool new_sb_mirror = false;
> >      > +
> >      > +    sb_mirror = shash_find_data(sb_mirrors, mirror_name);
> >      > +    if (!sb_mirror) {
> >      > +        sb_mirror = sbrec_mirror_insert(ovnsb_txn);
> >      > +        sbrec_mirror_set_name(sb_mirror, mirror_name);
> >      > +        shash_add(sb_mirrors, sb_mirror->name, sb_mirror);
> >      > +        new_sb_mirror = true;
> >      > +    }
> >      > +    sset_add(used_sb_mirrors, mirror_name);
> >      > +
> >      > +    if ((new_sb_mirror) || mirror_needs_update(nb_mirror,
> >     sb_mirror)) {
> >      > +        sbrec_mirror_set_filter(sb_mirror,nb_mirror->filter);
> >      > +        sbrec_mirror_set_index(sb_mirror, nb_mirror->index);
> >      > +        sbrec_mirror_set_sink(sb_mirror, nb_mirror->sink);
> >      > +        sbrec_mirror_set_type(sb_mirror, nb_mirror->type);
> >      > +    }
> >      > +}
> >      > +
> >      > +static void
> >      > +sync_mirrors(struct northd_input *input_data,
> >      > +            struct ovsdb_idl_txn *ovnsb_txn)
> >      > +{
> >      > +    struct shash sb_mirrors = SHASH_INITIALIZER(&sb_mirrors);
> >      > +    struct sset used_sb_mirrors =
> >     SSET_INITIALIZER(&used_sb_mirrors);
> >      > +
> >      > +    const struct sbrec_mirror *sb_mirror;
> >      > +    SBREC_MIRROR_TABLE_FOR_EACH (sb_mirror,
> >     input_data->sbrec_mirror_table) {
> >      > +        shash_add(&sb_mirrors, sb_mirror->name, sb_mirror);
> >      > +    }
> >      > +
> >      > +    const struct nbrec_mirror *nb_mirror;
> >      > +    NBREC_MIRROR_TABLE_FOR_EACH (nb_mirror,
> >     input_data->nbrec_mirror_table) {
> >      > +        sync_mirrors_iterate_nb_mirror(ovnsb_txn,
> >     nb_mirror->name, nb_mirror,
> >      > +                                     &sb_mirrors,
> &used_sb_mirrors);
> >      > +    }
> >      > +
> >      > +    const char *used_mirror;
> >      > +    const char *used_mirror_next;
> >      > +    SSET_FOR_EACH_SAFE (used_mirror, used_mirror_next,
> >     &used_sb_mirrors) {
> >      > +        shash_find_and_delete(&sb_mirrors, used_mirror);
> >      > +        sset_delete(&used_sb_mirrors,
> >     SSET_NODE_FROM_NAME(used_mirror));
> >      > +    }
> >      > +    sset_destroy(&used_sb_mirrors);
> >      > +
> >      > +    struct shash_node *node, *next;
> >      > +    SHASH_FOR_EACH_SAFE (node, next, &sb_mirrors) {
> >      > +        sbrec_mirror_delete(node->data);
> >      > +        shash_delete(&sb_mirrors, node);
> >      > +    }
> >      > +    shash_destroy(&sb_mirrors);
> >      > +}
> >
> >     The "used_mirrors" sset is unnecessary. The current algorithm is:
> >
> >     * Gather all SB mirrors into sb_mirrors shash
> >     * For each matching NB mirror:
> >          * Add mirror to sb_mirrors shash
> >          * Add mirror name to used_mirrors sset
> >     * For each mirror name in used_mirrors:
> >          * delete mirror from sb_mirrors
> >          * delete mirror name from used_mirrors
> >     * For each mirror in sb_mirrors:
> >          * delete mirror from SBDB
> >
> >     This can be simplified to:
> >
> >     * Gather all SB mirrors into sb_mirrors shash
> >     * For each matching NB mirror:
> >          * delete mirror from sb_mirrors
> >     * For each mirror in sb_mirrors:
> >          * delete mirror from SBDB
> >
> > Agreed. Will change this.
> >
> >      > +
> >      >   /*
> >      >    * struct 'dns_info' is used to sync the DNS records between
> >     OVN Northbound db
> >      >    * and Southbound db.
> >      > @@ -15794,6 +15965,7 @@ ovnnb_db_run(struct northd_input
> *input_data,
> >      >       sync_lbs(input_data, ovnsb_txn, &data->datapaths,
> &data->lbs);
> >      >       sync_port_groups(input_data, ovnsb_txn, &data->port_groups);
> >      >       sync_meters(input_data, ovnsb_txn, &data->meter_groups);
> >      > +    sync_mirrors(input_data, ovnsb_txn);
> >      >       sync_dns_entries(input_data, ovnsb_txn, &data->datapaths);
> >      >       cleanup_stale_fdb_entries(input_data, &data->datapaths);
> >      >       stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME,
> time_msec());
> >      > diff --git a/northd/northd.h b/northd/northd.h
> >      > index ea9bd5797..1670177ed 100644
> >      > --- a/northd/northd.h
> >      > +++ b/northd/northd.h
> >      > @@ -35,6 +35,7 @@ struct northd_input {
> >      >       const struct nbrec_acl_table *nbrec_acl_table;
> >      >       const struct nbrec_static_mac_binding_table
> >      >           *nbrec_static_mac_binding_table;
> >      > +    const struct nbrec_mirror_table *nbrec_mirror_table;
> >      >
> >      >       /* Southbound table references */
> >      >       const struct sbrec_sb_global_table *sbrec_sb_global_table;
> >      > @@ -53,6 +54,7 @@ struct northd_input {
> >      >       const struct sbrec_chassis_private_table
> >     *sbrec_chassis_private_table;
> >      >       const struct sbrec_static_mac_binding_table
> >      >           *sbrec_static_mac_binding_table;
> >      > +    const struct sbrec_mirror_table *sbrec_mirror_table;
> >      >
> >      >       /* Indexes */
> >      >       struct ovsdb_idl_index *sbrec_chassis_by_name;
> >      > diff --git a/tests/ovn-northd.at <http://ovn-northd.at>
> >     b/tests/ovn-northd.at <http://ovn-northd.at>
> >      > index 02c00c062..466d16916 100644
> >      > --- a/tests/ovn-northd.at <http://ovn-northd.at>
> >      > +++ b/tests/ovn-northd.at <http://ovn-northd.at>
> >      > @@ -2319,6 +2319,108 @@ check_meter_by_name NOT meter_me__${acl1}
> >     meter_me__${acl2}
> >      >   AT_CLEANUP
> >      >   ])
> >      >
> >      > +OVN_FOR_EACH_NORTHD_NO_HV([
> >      > +AT_SETUP([Check NB-SB mirrors sync])
> >      > +AT_KEYWORDS([mirrors])
> >      > +ovn_start
> >      > +
> >      > +check ovn-nbctl --wait=sb mirror-add mirror1 erspan 0 both
> >     10.10.10.2
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> >      > +"10.10.10.2"
> >      > +])
> >
> >     All of these AT_CHECK calls can use check_column instead. For the
> >     above,
> >     you can use
> >
> >     check_column 10.10.10.2 Mirror sink name=mirror1
> >
> >     Since there is only one row in the Mirror table, you might even be
> able
> >     to leave off that last bit.
> >
> >     check_column 10.10.10.2 Mirror sink
> >
> >     But I'm not 100% sure that will work.
> >
> > Sure, I will try out and change accordingly.
> >
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> >      > +erspan
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> >      > +0
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> >      > +both
> >      > +])
> >      > +
> >      > +check ovn-nbctl --wait=sb \
> >      > +    -- set mirror . sink=192.168.1.13
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> >      > +"192.168.1.13"
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> >      > +erspan
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> >      > +0
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> >      > +both
> >      > +])
> >      > +
> >      > +check ovn-nbctl --wait=sb \
> >      > +    -- set mirror . type=gre
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> >      > +gre
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> >      > +"192.168.1.13"
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> >      > +0
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> >      > +both
> >      > +])
> >      > +
> >      > +check ovn-nbctl --wait=sb \
> >      > +    -- set mirror . index=12
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> >      > +12
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> >      > +gre
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> >      > +"192.168.1.13"
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> >      > +both
> >      > +])
> >      > +
> >      > +check ovn-nbctl --wait=sb \
> >      > +    -- set mirror . filter=to-lport
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . filter], [0], [dnl
> >      > +to-lport
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . index], [0], [dnl
> >      > +12
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . type], [0], [dnl
> >      > +gre
> >      > +])
> >      > +
> >      > +AT_CHECK([ovn-sbctl get Mirror . Sink], [0], [dnl
> >      > +"192.168.1.13"
> >      > +])
> >      > +
> >      > +AT_CLEANUP
> >      > +])
> >
> >     This test is not very exhaustive. It should also:
> >         * Ensure that *all* mirrors are in both the NBDB and SBDB
> >         * Ensure that if a mirror is deleted from NBDB it is also deleted
> >     from SBDB
> >         * Try attaching mirrors to lsps and ensure that mirror_rules
> >     match in
> >     the SBDB as well. In addition, try adding/deleting mirror rules in
> the
> >     LSP and ensure that the mirror rules are represented in the SB
> >     Port_Bindings.
> >
> > We had not made this exhaustive here because we are checking
> > extensively in ovn.at <http://ovn.at> test cases (but ofcourse we
> verify
> > OVSDB there).
> > Since OVS DB can be correct only if OVN SB DB is correct.
> > Pretty much add, delete and attach (Also bulk) is covered there.
> > If you insist I can extend here as well.
> > Let me know if you feel it's important to be kept here as well.
>
> Ah I see the logic here. On the one hand, I agree with you since the
> coverage exists, and the data is implicitly checked.
>
> I think the utility of having explicit SB checks as part of this patch
> is that it can potentially help us to debug issues more quickly if they
> arise. Imagine that a later code change breaks port mirroring. If a test
> exists whose sole purpose is to ensure NB-SB database synchronisation,
> then the options are:
>
> 1) Both tests fail. Therefore the developer knows that they must have
> made a mistake somewhere in northd that is causing the mirroring code to
> not get synchronized with the SBDB. Furthermore, the test case will
> probably show clearly that the problem exists with a specific type of
> change to the NBDB. The developer can tweak just that part of the code
> and then try again.
>
> 2) Only the ovn.at test fails. The developer can then rule out the
> possibility of NB-SB synchronization being a problem and can focus
> solely on changes made in ovn-controller that appear to affect what gets
> set in OVS from the SBDB.
>
> However, if only the test in ovn.at exists, the developer has to debug
> all stages between the NBDB and OVS in order to try to determine at what
> stage the test is failing.
>
Understand the Intent. Sure. I will extend ovn-northd.at with the
add/delete,
attach/detach verification related to NB-SB sync tests here.

>
> >
> >
> >
> >      > +
> >      >   OVN_FOR_EACH_NORTHD_NO_HV([
> >      >   AT_SETUP([ACL skip hints for stateless config])
> >      >   AT_KEYWORDS([acl])
> >
> > Thanks & Regards,
> > Abhiram R N
>
> Thanks & Regards,
Abhiram R N