| Message ID | 20221114164820.88868-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Mark Michelson |
| Headers | show |
| Series | northd IP for address sets | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Hi Numan, I have just one minor suggestion below. On 11/14/22 11:48, numans@ovn.org wrote: > From: Numan Siddique <numans@ovn.org> > > 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 <numans@ovn.org> > --- > 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. > + } > + } > + > + 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 > +])
On Mon, Nov 14, 2022 at 3:23 PM Mark Michelson <mmichels@redhat.com> wrote: > > Hi Numan, I have just one minor suggestion below. > > On 11/14/22 11:48, numans@ovn.org wrote: > > From: Numan Siddique <numans@ovn.org> > > > > 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 <numans@ovn.org> > > --- > > 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 > > > + } > > + } > > + > > + 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 11/14/22 16:34, Numan Siddique wrote: > On Mon, Nov 14, 2022 at 3:23 PM Mark Michelson <mmichels@redhat.com> wrote: >> >> Hi Numan, I have just one minor suggestion below. >> >> On 11/14/22 11:48, numans@ovn.org wrote: >>> From: Numan Siddique <numans@ovn.org> >>> >>> 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 <numans@ovn.org> >>> --- >>> 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. > >> >>> + } >>> + } >>> + >>> + 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On Tue, Nov 15, 2022 at 11:10 AM Mark Michelson <mmichels@redhat.com> wrote: > > On 11/14/22 16:34, Numan Siddique wrote: > > On Mon, Nov 14, 2022 at 3:23 PM Mark Michelson <mmichels@redhat.com> wrote: > >> > >> Hi Numan, I have just one minor suggestion below. > >> > >> On 11/14/22 11:48, numans@ovn.org wrote: > >>> From: Numan Siddique <numans@ovn.org> > >>> > >>> 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 <numans@ovn.org> > >>> --- > >>> 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 > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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); + } + } + + 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 +])