On Tue, Nov 15, 2022 at 11:10 AM Mark Michelson <[email protected]> wrote: > > On 11/14/22 16:34, Numan Siddique wrote: > > On Mon, Nov 14, 2022 at 3:23 PM Mark Michelson <[email protected]> wrote: > >> > >> Hi Numan, I have just one minor suggestion below. > >> > >> On 11/14/22 11:48, [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..8a17998ec 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); > >>> + > >>> + if (addr_added && addr_added->n_values) { > >>> + for (size_t i = 0; i < addr_added->n_values; i++) { > >>> + struct ds ds = DS_EMPTY_INITIALIZER; > >>> + expr_constant_format(&addr_added->values[i], EXPR_C_INTEGER, > >>> &ds); > >>> + sbrec_address_set_update_addresses_addvalue(sb_as, > >>> ds_cstr(&ds)); > >>> + ds_destroy(&ds); > >> > >> Nit: Instead of creating and destroying a dynamic string in each loop > >> iteration, how about creating a single dynamic string and clearing it at > >> the beginning of each iteration? Then you can destroy the dynamic string > >> at the end of the function. I don't think it will have a huge impact on > >> performance, but it certainly should reduce the number of allocations. > > > > Thanks for the review. > > Sounds good to me. Shall I respin another version or wait for more > > comments ? > > > > Thanks > > Numan > > May as well respin a new version.
Done. v3 - https://patchwork.ozlabs.org/project/ovn/list/?series=328338 Numan > > > > >> > >>> + } > >>> + } > >>> + > >>> + if (addr_deleted && addr_deleted->n_values) { > >>> + for (size_t i = 0; i < addr_deleted->n_values; i++) { > >>> + struct ds ds = DS_EMPTY_INITIALIZER; > >>> + 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); > >>> > >>> 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 4f399eccb..f924dcfef 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]) > >>> +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 > >>> +]) > >> > >> _______________________________________________ > >> 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
