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