On Fri, Nov 18, 2022 at 11:27 AM Numan Siddique <[email protected]> wrote: > > ' > > On Fri, Nov 18, 2022 at 2:14 AM Han Zhou <[email protected]> wrote: > > > > On Tue, Nov 15, 2022 at 7:19 AM <[email protected]> wrote: > > > > > > From: Numan Siddique <[email protected]> > > > > > > Updates to NB address sets and NB port groups are handled > > > incrementally for syncing the SB address sets. This patch > > > doesn't support syncing the SB Address sets for the router > > > load balancer vips incrementally, instead a full recompute is > > > triggered for any changes to NB load balancers, NB load balancer > > > groups and NB logical routers. > > > > > > Signed-off-by: Numan Siddique <[email protected]> > > > --- > > > northd/en-sb-sync.c | 202 ++++++++++++++++++++++++++++++++++++--- > > > northd/en-sb-sync.h | 6 ++ > > > northd/inc-proc-northd.c | 18 +++- > > > tests/ovn-northd.at | 52 ++++++++++ > > > 4 files changed, 260 insertions(+), 18 deletions(-) > > > > > > diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c > > > index c3ba315df..e9ce3cec3 100644 > > > --- a/northd/en-sb-sync.c > > > +++ b/northd/en-sb-sync.c > > > @@ -22,6 +22,7 @@ > > > #include "openvswitch/util.h" > > > > > > #include "en-sb-sync.h" > > > +#include "include/ovn/expr.h" > > > #include "lib/inc-proc-eng.h" > > > #include "lib/lb.h" > > > #include "lib/ovn-nb-idl.h" > > > @@ -41,6 +42,13 @@ static void sync_address_sets(const struct > > nbrec_address_set_table *, > > > const struct sbrec_address_set_table *, > > > struct ovsdb_idl_txn *ovnsb_txn, > > > struct hmap *datapaths); > > > +static const struct sbrec_address_set *sb_address_set_lookup_by_name( > > > + struct ovsdb_idl_index *, const char *name); > > > +static void update_sb_addr_set(const char **nb_addresses, size_t > > n_addresses, > > > + const struct sbrec_address_set *); > > > +static void build_port_group_address_set(const struct nbrec_port_group *, > > > + struct svec *ipv4_addrs, > > > + struct svec *ipv6_addrs); > > > > > > void * > > > en_sb_sync_init(struct engine_node *node OVS_UNUSED, > > > @@ -94,6 +102,98 @@ en_address_set_sync_cleanup(void *data OVS_UNUSED) > > > > > > } > > > > > > +bool > > > +address_set_sync_nb_address_set_handler(struct engine_node *node > > OVS_UNUSED, > > > + void *data OVS_UNUSED) > > > +{ > > > + const struct nbrec_address_set_table *nb_address_set_table = > > > + EN_OVSDB_GET(engine_get_input("NB_address_set", node)); > > > + > > > + /* Return false if an address set is created or deleted. > > > + * Handle I-P for only updated address sets. */ > > > + const struct nbrec_address_set *nb_addr_set; > > > + NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (nb_addr_set, > > > + nb_address_set_table) { > > > + if (nbrec_address_set_is_new(nb_addr_set) || > > > + nbrec_address_set_is_deleted(nb_addr_set)) { > > > + return false; > > > + } > > > + } > > > + > > > + struct ovsdb_idl_index *sbrec_address_set_by_name = > > > + engine_ovsdb_node_get_index( > > > + engine_get_input("SB_address_set", node), > > > + "sbrec_address_set_by_name"); > > > + > > > + NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (nb_addr_set, > > > + nb_address_set_table) { > > > + const struct sbrec_address_set *sb_addr_set = > > > + sb_address_set_lookup_by_name(sbrec_address_set_by_name, > > > + nb_addr_set->name); > > > + if (!sb_addr_set) { > > > + return false; > > > + } > > > + update_sb_addr_set((const char **) nb_addr_set->addresses, > > > + nb_addr_set->n_addresses, sb_addr_set); > > > + } > > > + > > > + return true; > > > +} > > > + > > > +bool > > > +address_set_sync_nb_port_group_handler(struct engine_node *node > > OVS_UNUSED, > > > + void *data OVS_UNUSED) > > > +{ > > > + const struct nbrec_port_group *nb_pg; > > > + const struct nbrec_port_group_table *nb_port_group_table = > > > + EN_OVSDB_GET(engine_get_input("NB_port_group", node)); > > > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, nb_port_group_table) > > { > > > + if (nbrec_port_group_is_new(nb_pg) || > > > + nbrec_port_group_is_deleted(nb_pg)) { > > > + return false; > > > + } > > > + } > > > + > > > + struct ovsdb_idl_index *sbrec_address_set_by_name = > > > + engine_ovsdb_node_get_index( > > > + engine_get_input("SB_address_set", node), > > > + "sbrec_address_set_by_name"); > > > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, nb_port_group_table) > > { > > > + char *ipv4_addrs_name = xasprintf("%s_ip4", nb_pg->name); > > > + const struct sbrec_address_set *sb_addr_set_v4 = > > > + sb_address_set_lookup_by_name(sbrec_address_set_by_name, > > > + ipv4_addrs_name); > > > + if (!sb_addr_set_v4) { > > > + free(ipv4_addrs_name); > > > + return false; > > > + } > > > + char *ipv6_addrs_name = xasprintf("%s_ip6", nb_pg->name); > > > + const struct sbrec_address_set *sb_addr_set_v6 = > > > + sb_address_set_lookup_by_name(sbrec_address_set_by_name, > > > + ipv6_addrs_name); > > > + if (!sb_addr_set_v6) { > > > + free(ipv4_addrs_name); > > > + free(ipv6_addrs_name); > > > + return false; > > > + } > > > + > > > + struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > > > + struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > > > + build_port_group_address_set(nb_pg, &ipv4_addrs, &ipv6_addrs); > > > + update_sb_addr_set((const char **) ipv4_addrs.names, > > ipv4_addrs.n, > > > + sb_addr_set_v4); > > > + update_sb_addr_set((const char **) ipv6_addrs.names, > > ipv6_addrs.n, > > > + sb_addr_set_v6); > > > + > > > + free(ipv4_addrs_name); > > > + free(ipv6_addrs_name); > > > + svec_destroy(&ipv4_addrs); > > > + svec_destroy(&ipv6_addrs); > > > + } > > > + > > > + return true; > > > +} > > > + > > > /* static functions. */ > > > static void > > > sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > > > @@ -106,10 +206,11 @@ sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, > > const char *name, > > > if (!sb_address_set) { > > > sb_address_set = sbrec_address_set_insert(ovnsb_txn); > > > sbrec_address_set_set_name(sb_address_set, name); > > > + sbrec_address_set_set_addresses(sb_address_set, > > > + addrs, n_addrs); > > > + } else { > > > + update_sb_addr_set(addrs, n_addrs, sb_address_set); > > > } > > > - > > > - sbrec_address_set_set_addresses(sb_address_set, > > > - addrs, n_addrs); > > > } > > > > > > /* OVN_Southbound Address_Set table contains same records as in north > > > @@ -148,18 +249,7 @@ sync_address_sets( > > > nb_port_group_table) { > > > struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > > > struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > > > - for (size_t i = 0; i < nb_port_group->n_ports; i++) { > > > - for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; > > j++) { > > > - const char *addrs = > > nb_port_group->ports[i]->addresses[j]; > > > - if (!is_dynamic_lsp_address(addrs)) { > > > - split_addresses(addrs, &ipv4_addrs, &ipv6_addrs); > > > - } > > > - } > > > - if (nb_port_group->ports[i]->dynamic_addresses) { > > > - > > split_addresses(nb_port_group->ports[i]->dynamic_addresses, > > > - &ipv4_addrs, &ipv6_addrs); > > > - } > > > - } > > > + build_port_group_address_set(nb_port_group, &ipv4_addrs, > > &ipv6_addrs); > > > char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name); > > > char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name); > > > sync_address_set(ovnsb_txn, ipv4_addrs_name, > > > @@ -228,3 +318,85 @@ sync_address_sets( > > > } > > > shash_destroy(&sb_address_sets); > > > } > > > + > > > +static void > > > +update_sb_addr_set(const char **nb_addresses, size_t n_addresses, > > > + const struct sbrec_address_set *sb_as) > > > +{ > > > + struct expr_constant_set *cs_nb_as = > > > + expr_constant_set_create_integers( > > > + (const char *const *) nb_addresses, n_addresses); > > > + struct expr_constant_set *cs_sb_as = > > > + expr_constant_set_create_integers( > > > + (const char *const *) sb_as->addresses, sb_as->n_addresses); > > > + > > > + struct expr_constant_set *addr_added = NULL; > > > + struct expr_constant_set *addr_deleted = NULL; > > > + expr_constant_set_integers_diff(cs_sb_as, cs_nb_as, &addr_added, > > > + &addr_deleted); > > > + > > > + struct ds ds = DS_EMPTY_INITIALIZER; > > > + if (addr_added && addr_added->n_values) { > > > + for (size_t i = 0; i < addr_added->n_values; i++) { > > > + ds_clear(&ds); > > > + expr_constant_format(&addr_added->values[i], EXPR_C_INTEGER, > > &ds); > > > + sbrec_address_set_update_addresses_addvalue(sb_as, > > ds_cstr(&ds)); > > > + } > > > + } > > > + > > > + if (addr_deleted && addr_deleted->n_values) { > > > + for (size_t i = 0; i < addr_deleted->n_values; i++) { > > > + ds_clear(&ds); > > > + expr_constant_format(&addr_deleted->values[i], > > > + EXPR_C_INTEGER, &ds); > > > + sbrec_address_set_update_addresses_delvalue(sb_as, > > ds_cstr(&ds)); > > > + } > > > + } > > > + > > > + ds_destroy(&ds); > > > + expr_constant_set_destroy(cs_nb_as); > > > + free(cs_nb_as); > > > + expr_constant_set_destroy(cs_sb_as); > > > + free(cs_sb_as); > > > + expr_constant_set_destroy(addr_added); > > > + free(addr_added); > > > + expr_constant_set_destroy(addr_deleted); > > > + free(addr_deleted); > > > +} > > > + > > > +static void > > > +build_port_group_address_set(const struct nbrec_port_group > > *nb_port_group, > > > + struct svec *ipv4_addrs, > > > + struct svec *ipv6_addrs) > > > +{ > > > + for (size_t i = 0; i < nb_port_group->n_ports; i++) { > > > + for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; > > j++) { > > > + const char *addrs = nb_port_group->ports[i]->addresses[j]; > > > + if (!is_dynamic_lsp_address(addrs)) { > > > + split_addresses(addrs, ipv4_addrs, ipv6_addrs); > > > + } > > > + } > > > + if (nb_port_group->ports[i]->dynamic_addresses) { > > > + split_addresses(nb_port_group->ports[i]->dynamic_addresses, > > > + ipv4_addrs, ipv6_addrs); > > > + } > > > + } > > > +} > > > + > > > +/* Finds and returns the address set with the given 'name', or NULL if > > no such > > > + * address set exists. */ > > > +static const struct sbrec_address_set * > > > +sb_address_set_lookup_by_name(struct ovsdb_idl_index > > *sbrec_addr_set_by_name, > > > + const char *name) > > > +{ > > > + struct sbrec_address_set *target = sbrec_address_set_index_init_row( > > > + sbrec_addr_set_by_name); > > > + sbrec_address_set_index_set_name(target, name); > > > + > > > + struct sbrec_address_set *retval = sbrec_address_set_index_find( > > > + sbrec_addr_set_by_name, target); > > > + > > > + sbrec_address_set_index_destroy_row(target); > > > + > > > + return retval; > > > +} > > > diff --git a/northd/en-sb-sync.h b/northd/en-sb-sync.h > > > index f99d6a9fc..a63453fe5 100644 > > > --- a/northd/en-sb-sync.h > > > +++ b/northd/en-sb-sync.h > > > @@ -3,12 +3,18 @@ > > > > > > #include "lib/inc-proc-eng.h" > > > > > > +/* en_sb_sync engine node functions. */ > > > void *en_sb_sync_init(struct engine_node *, struct engine_arg *); > > > void en_sb_sync_run(struct engine_node *, void *data); > > > void en_sb_sync_cleanup(void *data); > > > > > > +/* en_address_set_sync engine node functions. */ > > > void *en_address_set_sync_init(struct engine_node *, struct engine_arg > > *); > > > void en_address_set_sync_run(struct engine_node *, void *data); > > > void en_address_set_sync_cleanup(void *data); > > > +bool address_set_sync_nb_address_set_handler(struct engine_node *, > > > + void *data); > > > +bool address_set_sync_nb_port_group_handler(struct engine_node *, > > > + void *data); > > > > > > #endif > > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > > index b48f53f17..e2c25046a 100644 > > > --- a/northd/inc-proc-northd.c > > > +++ b/northd/inc-proc-northd.c > > > @@ -238,8 +238,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > * the NB database tables. > > > * Right now this engine only syncs the SB Address_Set table. > > > */ > > > - engine_add_input(&en_address_set_sync, &en_nb_address_set, NULL); > > > - engine_add_input(&en_address_set_sync, &en_nb_port_group, NULL); > > > + engine_add_input(&en_address_set_sync, &en_nb_address_set, > > > + address_set_sync_nb_address_set_handler); > > > + engine_add_input(&en_address_set_sync, &en_nb_port_group, > > > + address_set_sync_nb_port_group_handler); > > > engine_add_input(&en_address_set_sync, &en_nb_load_balancer, NULL); > > > engine_add_input(&en_address_set_sync, &en_nb_load_balancer_group, > > NULL); > > > engine_add_input(&en_address_set_sync, &en_nb_logical_router, NULL); > > > @@ -248,8 +250,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > > > > /* We need the en_northd generated data as input to > > en_address_set_sync > > > * node to access the data generated by it (eg. struct ovn_datapath). > > > + * The handler is noop since en_northd always falls back to full > > recompute > > > + * (since it has no input handlers) and it doesn't yet indicate what > > > + * changed. It doesn't make sense to add NULL handler for this input, > > > + * otherwise 'en_address_set_sync' will always fall back to full > > recompute. > > > */ > > > - engine_add_input(&en_address_set_sync, &en_northd, NULL); > > > + engine_add_input(&en_address_set_sync, &en_northd, > > engine_noop_handler); > > > > I don't think we should use noop handler here. en_address_set_sync clearly > > depends on en_northd. So if en_northd changes (e.g. ovn_datapath changes), > > we do want the en_address_set_sync node to get recomputed. > > And I understand that the purpose of this patch is to make sure when > > address_set changes, we don't want to trigger any recompute. > > Not really. If there are any NB address set changes, it would still > result in recompute of en_northd engine node and "en_lflow" engine > nodes. > > This patch attempts to not recompute the NB to SB sync of address sets. > > > In that case, > > we should remove the dependency between en_nb_address_set -> en_northd (if > > en_northd doesn't really depend on NB address_set). For the other > > dependencies such as NB port_group, we should examine them the same way: > > try to remove the dependency for en_northd, or implement I-P handler in > > en_northd for those inputs, and make sure an input can be I-P handled by > > all nodes depending on it. Otherwise, recompute is anyway needed. > > In the current implementation, if I read the code correctly, it still > > triggers recompute even with this noop_handler, because en_northd depends > > on those inputs and it doesn't implement any I-P handler. > > Syncing SB address sets depends on 3 main inputs > > 1 . NB Address sets -> To sync NB address sets to SB address sets. > This doesn't depend on the en_northd engine data. > > 2. NB Port groups. To sync the SB Address sets which correspond to > NB Port groups (which northd generates). > This doesn't depend on the "en_northd" engine data. > > 3. NB Logical router / NB Logical load balancer / NB load balancer > group -> To sync the SB address sets generated for the routable load > balancer VIPs linked to a logical router. > This does depend on the "datapaths" hmap of en_northd engine data. >
The reason I added a noop handler for en_northd input is because of the above 3 conditions to sync SB address sets, The patch series adds the below inputs engine_add_input(&en_address_set_sync, &en_nb_address_set, address_set_sync_nb_address_set_handler); engine_add_input(&en_address_set_sync, &en_nb_port_group, address_set_sync_nb_port_group_handler); engine_add_input(&en_address_set_sync, &en_nb_load_balancer, NULL); engine_add_input(&en_address_set_sync, &en_nb_load_balancer_group, NULL); engine_add_input(&en_address_set_sync, &en_nb_logical_router, NULL); engine_add_input(&en_address_set_sync, &en_sb_address_set, engine_noop_handler); engine_add_input(&en_address_set_sync, &en_northd, engine_noop_handler); Since (3) is handled by en_nb_load_balancer, en_nb_load_balancer_group and en_nb_logical_router inputs, I thought it is fine to have a noop handler for en_northd and we add en_northd to only access the 'datapaths' data of en_northd engine. I do remember we had similar conversations earlier with en_runtime_data input for en_lflow_output_handler. I still think it is ok to have a noop handler and to document it (which this patch does). Thanks Numan > > In v4, I'll add the handler (address_set_sync_northd_handler) for > en_northd engine input to the 'en_address_set_sync' node. Since > en_northd engine doesn't have handlers for its input nodes, it is not > possible to determine > what changed. So the handler address_set_sync_northd_handler() will > loop through all the datapaths and sync the SB address sets for > router reachable vips. > > Whenever the en_northd engine is updated (which is the case for any NB > db change), this handler will be invoked (even though there is no real > need to sync the SB address sets). But I think that is ok. > > Please let me know what you think. > > > > > > > > > > engine_add_input(&en_sb_sync, &en_address_set_sync, NULL); > > > engine_add_input(&en_northd_output, &en_sb_sync, > > > @@ -300,6 +306,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > > engine_ovsdb_node_add_index(&en_sb_mac_binding, > > > "sbrec_mac_binding_by_datapath", > > > sbrec_mac_binding_by_datapath); > > > + > > > + struct ovsdb_idl_index *sbrec_address_set_by_name > > > + = ovsdb_idl_index_create1(sb->idl, &sbrec_address_set_col_name); > > > + engine_ovsdb_node_add_index(&en_sb_address_set, > > > + "sbrec_address_set_by_name", > > > + sbrec_address_set_by_name); > > > } > > > > > > void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > index e849afd85..06de90c80 100644 > > > --- a/tests/ovn-northd.at > > > +++ b/tests/ovn-northd.at > > > @@ -7929,3 +7929,55 @@ AT_CHECK([grep "lr_in_arp_resolve" R1flows | grep > > priority=90 | sort], [0], [dnl > > > > > > AT_CLEANUP > > > ]) > > > + > > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > > +AT_SETUP([Address set incremental processing]) > > > > The test case seems only checking if it is using mutation, but doesn't > > check if ovn-northd did recompute or I-P. I think it should check whether > > it has performed recompute, probably using inc-engine/show-stats. > > Ack. > > Thanks > Numan > > > > > Thanks, > > Han > > > > > +ovn_start > > > + > > > +foo_as_uuid=$(ovn-nbctl create address_set name=foo > > addresses=\"1.1.1.1\",\"1.1.1.2\") > > > +ovn-nbctl --wait=sb sync > > > + > > > +check_column '1.1.1.1 1.1.1.2' Address_Set addresses name=foo > > > + > > > +rm -f northd/ovn-northd.log > > > +check as northd ovn-appctl -t NORTHD_TYPE vlog/reopen > > > +check as northd ovn-appctl -t NORTHD_TYPE vlog/set jsonrpc:dbg > > > + > > > +check ovn-nbctl --wait=sb add address_set $foo_as_uuid addresses 1.1.1.3 > > > +check_column '1.1.1.1 1.1.1.2 1.1.1.3' Address_Set addresses name=foo > > > + > > > +AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \ > > > +grep -c mutate], [0], [1 > > > +]) > > > + > > > +check ovn-nbctl --wait=sb add address_set $foo_as_uuid addresses \ > > > +1.1.1.4 -- remove address_set $foo_as_uuid addresses 1.1.1.1 > > > +check_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set addresses name=foo > > > + > > > +AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \ > > > +grep -c mutate], [0], [2 > > > +]) > > > + > > > +# Pause ovn-northd and add/remove few addresses. when it is resumed > > > +# it should use mutate for updating the address sets. > > > +check as northd ovn-appctl -t NORTHD_TYPE pause > > > +check as northd-backup ovn-appctl -t NORTHD_TYPE pause > > > + > > > +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.5 > > > +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.6 > > > +check ovn-nbctl remove address_set $foo_as_uuid addresses 1.1.1.2 > > > + > > > +check_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set addresses name=foo > > > + > > > +# Resume northd now > > > +check as northd ovn-appctl -t NORTHD_TYPE resume > > > +check ovn-nbctl --wait=sb sync > > > + > > > +check_column '1.1.1.3 1.1.1.4 1.1.1.5 1.1.1.6' Address_Set addresses > > name=foo > > > + > > > +AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \ > > > +grep -c mutate], [0], [3 > > > +]) > > > + > > > +AT_CLEANUP > > > +]) > > > -- > > > 2.38.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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
