Thank you. Looks like Mark is a lot better at communication and explained better why the approach taken before was over complicated and error prone. :)
Thanks, I love the simplicity of the new version, and the test cases no longer need to be as complicated anymore. Acked-By: Ihar Hrachyshka <[email protected]> On Thu, Dec 1, 2022 at 12:05 PM Abhiram R N <[email protected]> wrote: > > Changes which syncs the NB port mirrors with SB port mirrors. > Also syncs mirror_rules column in Logical_Switch_Port table > of NB DB with corresponding mirror_rules column in > Port_Binding table of SB DB. > Further test added to check the NB and SB sync > > Co-authored-by: Veda Barrenkala <[email protected]> > Signed-off-by: Veda Barrenkala <[email protected]> > Signed-off-by: Abhiram R N <[email protected]> > --- > v17-->v18: Addressed comments of Mark from v17 > > Modifications related to comments are in > northd.c, ovn-northd.at > Other files changed to just fix rebase conflicts > > northd/en-northd.c | 4 ++ > northd/inc-proc-northd.c | 4 ++ > northd/northd.c | 133 +++++++++++++++++++++++++++++++++++++++ > northd/northd.h | 2 + > tests/ovn-northd.at | 105 +++++++++++++++++++++++++++++++ > 5 files changed, 248 insertions(+) > > diff --git a/northd/en-northd.c b/northd/en-northd.c > index 9360c68e9..09fe8976a 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -80,6 +80,8 @@ void en_northd_run(struct engine_node *node, void *data) > EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node)); > input_data.nbrec_chassis_template_var_table = > EN_OVSDB_GET(engine_get_input("NB_chassis_template_var", 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)); > @@ -113,6 +115,8 @@ void en_northd_run(struct engine_node *node, void *data) > EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node)); > input_data.sbrec_chassis_template_var_table = > EN_OVSDB_GET(engine_get_input("SB_chassis_template_var", 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 ff3620d62..363e384bd 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") \ > @@ -95,6 +96,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") \ > @@ -178,6 +180,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); > @@ -200,6 +203,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 74facce7a..66a6757a2 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3238,6 +3238,60 @@ ovn_port_update_sbrec_chassis( > free(requested_chassis_sb); > } > > +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, > @@ -3597,6 +3651,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); > @@ -15357,6 +15420,75 @@ 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) > +{ > + 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; > + } > + > + 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); > + > + 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); > + shash_find_and_delete(&sb_mirrors, nb_mirror->name); > + } > + > + 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); > +} > + > /* > * struct 'dns_info' is used to sync the DNS records between OVN Northbound > db > * and Southbound db. > @@ -16027,6 +16159,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); > sync_template_vars(input_data, ovnsb_txn); > > diff --git a/northd/northd.h b/northd/northd.h > index 7942c0a34..ff8727cb7 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -37,6 +37,7 @@ struct northd_input { > *nbrec_static_mac_binding_table; > const struct nbrec_chassis_template_var_table > *nbrec_chassis_template_var_table; > + const struct nbrec_mirror_table *nbrec_mirror_table; > > /* Southbound table references */ > const struct sbrec_sb_global_table *sbrec_sb_global_table; > @@ -57,6 +58,7 @@ struct northd_input { > *sbrec_static_mac_binding_table; > const struct sbrec_chassis_template_var_table > *sbrec_chassis_template_var_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 9a76ca340..6af62c7ae 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -2326,6 +2326,111 @@ 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 ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0-port1 > +check ovn-nbctl lsp-add sw0 sw0-port2 > +check ovn-nbctl --wait=sb mirror-add mirror1 erspan 1 both 10.10.10.2 > +check_column mirror1 Mirror name > +check_column 10.10.10.2 Mirror sink > +check_column erspan Mirror type > +check_column 1 Mirror index > +check_column both Mirror filter > + > +check ovn-nbctl --wait=sb set mirror . sink=192.168.1.13 > + > +check_column 192.168.1.13 Mirror sink > +check_column erspan Mirror type > +check_column 1 Mirror index > +check_column both Mirror filter > + > +check ovn-nbctl --wait=sb set mirror . type=gre > + > +check_column 192.168.1.13 Mirror sink > +check_column gre Mirror type > +check_column 1 Mirror index > +check_column both Mirror filter > + > +check ovn-nbctl --wait=sb set mirror . index=12 > + > +check_column 192.168.1.13 Mirror sink > +check_column gre Mirror type > +check_column 12 Mirror index > +check_column both Mirror filter > + > +check ovn-nbctl --wait=sb set mirror . filter=to-lport > + > +check_column 192.168.1.13 Mirror sink > +check_column gre Mirror type > +check_column 12 Mirror index > +check_column to-lport Mirror filter > + > +# Verify mirror attach > +check ovn-nbctl --wait=sb lsp-attach-mirror sw0-port1 mirror1 > + > +mirror1uuid=$(fetch_column sb:Mirror _uuid name=mirror1) > +check_column "$mirror1uuid" sb:Port_Binding mirror_rules > logical_port=sw0-port1 > + > +check ovn-nbctl --wait=sb mirror-add mirror2 gre 2 both 10.10.10.2 > +check_row_count sb:Mirror 2 > + > +# Verify mirror detach (and another attach) > +check ovn-nbctl lsp-attach-mirror sw0-port1 mirror2 > +check ovn-nbctl lsp-detach-mirror sw0-port1 mirror1 > +check ovn-nbctl --wait=sb sync > + > +mirror2uuid=$(fetch_column sb:Mirror _uuid name=mirror2) > +check_column "$mirror2uuid" sb:Port_Binding mirror_rules > logical_port=sw0-port1 > + > +# Verify mirror-del (one by one) > +check ovn-nbctl --wait=sb mirror-del mirror2 > +check_row_count sb:Mirror 1 > +check ovn-nbctl --wait=sb mirror-del mirror1 > +check_row_count sb:Mirror 0 > +check_column "" sb:Port_Binding mirror_rules logical_port=sw0-port1 > + > +# Verify mirror-add > +check ovn-nbctl --wait=sb mirror-add mirror2 gre 2 both 10.10.10.2 > +check_row_count sb:Mirror 1 > + > +check_column 10.10.10.2 Mirror sink > +check_column gre Mirror type > +check_column 2 Mirror index > +check_column both Mirror filter > + > +# Verify same attached to multiple ports > +check ovn-nbctl --wait=sb lsp-attach-mirror sw0-port1 mirror2 > +check ovn-nbctl --wait=sb lsp-attach-mirror sw0-port2 mirror2 > + > +mirror2uuid=$(fetch_column sb:Mirror _uuid name=mirror2) > +check_column "$mirror2uuid" sb:Port_Binding mirror_rules > logical_port=sw0-port1 > +check_column "$mirror2uuid" sb:Port_Binding mirror_rules > logical_port=sw0-port2 > + > +# Verify same port attached to multiple mirrors > +check ovn-nbctl --wait=sb mirror-add mirror1 erspan 1 both 10.10.10.2 > +check ovn-nbctl --wait=sb lsp-attach-mirror sw0-port1 mirror1 > +check_row_count sb:Mirror 2 > +check_row_count nb:Mirror 2 > + > +mirror1uuid=$(fetch_column sb:Mirror _uuid name=mirror1) > +check_column "$mirror2uuid $mirror1uuid" sb:Port_Binding mirror_rules > logical_port=sw0-port1 > + > +# Verify delete (bulk) > +check ovn-nbctl --wait=sb mirror-del > +check_row_count nb:Mirror 0 > +check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port1 > +check_column "" nb:Logical_Switch_Port mirror_rules name=sw0-port2 > +check_row_count sb:Mirror 0 > +check_column "" sb:Port_Binding mirror_rules logical_port=sw0-port1 > +check_column "" sb:Port_Binding mirror_rules logical_port=sw0-port2 > + > +AT_CLEANUP > +]) > + > OVN_FOR_EACH_NORTHD_NO_HV([ > AT_SETUP([ACL skip hints for stateless config]) > AT_KEYWORDS([acl]) > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
