On Fri, Nov 18, 2022 at 7:38 PM Han Zhou <[email protected]> wrote: > > On Fri, Nov 18, 2022 at 3:53 PM Han Zhou <[email protected]> wrote: > > > > > > > > On Fri, Nov 18, 2022 at 2:58 PM Numan Siddique <[email protected]> wrote: > > > > > > ' > > > > > > On Fri, Nov 18, 2022 at 5:44 PM Numan Siddique <[email protected]> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, Nov 18, 2022, 4:55 PM Han Zhou <[email protected]> wrote: > > > >> > > > >> On Fri, Nov 18, 2022 at 1:47 PM Numan Siddique <[email protected]> > wrote: > > > >> > > > > >> > On Fri, Nov 18, 2022 at 3:46 PM Han Zhou <[email protected]> wrote: > > > >> > > > > > >> > > On Fri, Nov 18, 2022 at 11:23 AM Numan Siddique <[email protected]> > wrote: > > > >> > > > > > > >> > > > On Fri, Nov 18, 2022 at 11:27 AM Numan Siddique <[email protected] > > > > > >> wrote: > > > >> > > > > > > > >> > > > > ' > > > >> > > > > > > > >> > > > > On Fri, Nov 18, 2022 at 2:14 AM Han Zhou <[email protected]> > wrote: > > > >> > > > > > > > > >> > > > > > On Tue, Nov 15, 2022 at 7:19 AM <[email protected]> wrote: > > > >> > > > > > > > > > >> > > > > > > From: Numan Siddique <[email protected]> > > > >> > > > > > > > > > >> > > > > > > Updates to NB address sets and NB port groups are handled > > > >> > > > > > > incrementally for syncing the SB address sets. This > patch > > > >> > > > > > > doesn't support syncing the SB Address sets for the > router > > > >> > > > > > > load balancer vips incrementally, instead a full > recompute is > > > >> > > > > > > triggered for any changes to NB load balancers, NB load > balancer > > > >> > > > > > > groups and NB logical routers. > > > >> > > > > > > > > > >> > > > > > > Signed-off-by: Numan Siddique <[email protected]> > > > >> > > > > > > --- > > > >> > > > > > > northd/en-sb-sync.c | 202 > > > >> > > ++++++++++++++++++++++++++++++++++++--- > > > >> > > > > > > northd/en-sb-sync.h | 6 ++ > > > >> > > > > > > northd/inc-proc-northd.c | 18 +++- > > > >> > > > > > > tests/ovn-northd.at | 52 ++++++++++ > > > >> > > > > > > 4 files changed, 260 insertions(+), 18 deletions(-) > > > >> > > > > > > > > > >> > > > > > > diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c > > > >> > > > > > > index c3ba315df..e9ce3cec3 100644 > > > >> > > > > > > --- a/northd/en-sb-sync.c > > > >> > > > > > > +++ b/northd/en-sb-sync.c > > > >> > > > > > > @@ -22,6 +22,7 @@ > > > >> > > > > > > #include "openvswitch/util.h" > > > >> > > > > > > > > > >> > > > > > > #include "en-sb-sync.h" > > > >> > > > > > > +#include "include/ovn/expr.h" > > > >> > > > > > > #include "lib/inc-proc-eng.h" > > > >> > > > > > > #include "lib/lb.h" > > > >> > > > > > > #include "lib/ovn-nb-idl.h" > > > >> > > > > > > @@ -41,6 +42,13 @@ static void sync_address_sets(const > struct > > > >> > > > > > nbrec_address_set_table *, > > > >> > > > > > > const struct > > > >> sbrec_address_set_table > > > >> > > *, > > > >> > > > > > > struct ovsdb_idl_txn > *ovnsb_txn, > > > >> > > > > > > struct hmap *datapaths); > > > >> > > > > > > +static const struct sbrec_address_set > > > >> > > *sb_address_set_lookup_by_name( > > > >> > > > > > > + struct ovsdb_idl_index *, const char *name); > > > >> > > > > > > +static void update_sb_addr_set(const char > **nb_addresses, > > > >> size_t > > > >> > > > > > n_addresses, > > > >> > > > > > > + const struct > sbrec_address_set > > > >> *); > > > >> > > > > > > +static void build_port_group_address_set(const struct > > > >> > > nbrec_port_group *, > > > >> > > > > > > + struct svec > > > >> *ipv4_addrs, > > > >> > > > > > > + struct svec > > > >> *ipv6_addrs); > > > >> > > > > > > > > > >> > > > > > > void * > > > >> > > > > > > en_sb_sync_init(struct engine_node *node OVS_UNUSED, > > > >> > > > > > > @@ -94,6 +102,98 @@ en_address_set_sync_cleanup(void > *data > > > >> > > OVS_UNUSED) > > > >> > > > > > > > > > >> > > > > > > } > > > >> > > > > > > > > > >> > > > > > > +bool > > > >> > > > > > > +address_set_sync_nb_address_set_handler(struct > engine_node > > > >> *node > > > >> > > > > > OVS_UNUSED, > > > >> > > > > > > + void *data > OVS_UNUSED) > > > >> > > > > > > +{ > > > >> > > > > > > + const struct nbrec_address_set_table > *nb_address_set_table > > > >> = > > > >> > > > > > > + EN_OVSDB_GET(engine_get_input("NB_address_set", > node)); > > > >> > > > > > > + > > > >> > > > > > > + /* Return false if an address set is created or > deleted. > > > >> > > > > > > + * Handle I-P for only updated address sets. */ > > > >> > > > > > > + const struct nbrec_address_set *nb_addr_set; > > > >> > > > > > > + NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED > (nb_addr_set, > > > >> > > > > > > + > > > >> > > nb_address_set_table) { > > > >> > > > > > > + if (nbrec_address_set_is_new(nb_addr_set) || > > > >> > > > > > > + > nbrec_address_set_is_deleted(nb_addr_set)) { > > > >> > > > > > > + return false; > > > >> > > > > > > + } > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + struct ovsdb_idl_index *sbrec_address_set_by_name = > > > >> > > > > > > + engine_ovsdb_node_get_index( > > > >> > > > > > > + engine_get_input("SB_address_set", > node), > > > >> > > > > > > + "sbrec_address_set_by_name"); > > > >> > > > > > > + > > > >> > > > > > > + NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED > (nb_addr_set, > > > >> > > > > > > + > > > >> > > nb_address_set_table) { > > > >> > > > > > > + const struct sbrec_address_set *sb_addr_set = > > > >> > > > > > > + > > > >> > > sb_address_set_lookup_by_name(sbrec_address_set_by_name, > > > >> > > > > > > + > nb_addr_set->name); > > > >> > > > > > > + if (!sb_addr_set) { > > > >> > > > > > > + return false; > > > >> > > > > > > + } > > > >> > > > > > > + update_sb_addr_set((const char **) > > > >> nb_addr_set->addresses, > > > >> > > > > > > + nb_addr_set->n_addresses, > > > >> sb_addr_set); > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + return true; > > > >> > > > > > > +} > > > >> > > > > > > + > > > >> > > > > > > +bool > > > >> > > > > > > +address_set_sync_nb_port_group_handler(struct > engine_node *node > > > >> > > > > > OVS_UNUSED, > > > >> > > > > > > + void *data > OVS_UNUSED) > > > >> > > > > > > +{ > > > >> > > > > > > + const struct nbrec_port_group *nb_pg; > > > >> > > > > > > + const struct nbrec_port_group_table > *nb_port_group_table = > > > >> > > > > > > + EN_OVSDB_GET(engine_get_input("NB_port_group", > node)); > > > >> > > > > > > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, > > > >> > > nb_port_group_table) > > > >> > > > > > { > > > >> > > > > > > + if (nbrec_port_group_is_new(nb_pg) || > > > >> > > > > > > + nbrec_port_group_is_deleted(nb_pg)) { > > > >> > > > > > > + return false; > > > >> > > > > > > + } > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + struct ovsdb_idl_index *sbrec_address_set_by_name = > > > >> > > > > > > + engine_ovsdb_node_get_index( > > > >> > > > > > > + engine_get_input("SB_address_set", > node), > > > >> > > > > > > + "sbrec_address_set_by_name"); > > > >> > > > > > > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, > > > >> > > nb_port_group_table) > > > >> > > > > > { > > > >> > > > > > > + char *ipv4_addrs_name = xasprintf("%s_ip4", > > > >> nb_pg->name); > > > >> > > > > > > + const struct sbrec_address_set *sb_addr_set_v4 = > > > >> > > > > > > + > > > >> > > sb_address_set_lookup_by_name(sbrec_address_set_by_name, > > > >> > > > > > > + > ipv4_addrs_name); > > > >> > > > > > > + if (!sb_addr_set_v4) { > > > >> > > > > > > + free(ipv4_addrs_name); > > > >> > > > > > > + return false; > > > >> > > > > > > + } > > > >> > > > > > > + char *ipv6_addrs_name = xasprintf("%s_ip6", > > > >> nb_pg->name); > > > >> > > > > > > + const struct sbrec_address_set *sb_addr_set_v6 = > > > >> > > > > > > + > > > >> > > sb_address_set_lookup_by_name(sbrec_address_set_by_name, > > > >> > > > > > > + > ipv6_addrs_name); > > > >> > > > > > > + if (!sb_addr_set_v6) { > > > >> > > > > > > + free(ipv4_addrs_name); > > > >> > > > > > > + free(ipv6_addrs_name); > > > >> > > > > > > + return false; > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > > > >> > > > > > > + struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > > > >> > > > > > > + build_port_group_address_set(nb_pg, &ipv4_addrs, > > > >> > > &ipv6_addrs); > > > >> > > > > > > + update_sb_addr_set((const char **) > ipv4_addrs.names, > > > >> > > > > > ipv4_addrs.n, > > > >> > > > > > > + sb_addr_set_v4); > > > >> > > > > > > + update_sb_addr_set((const char **) > ipv6_addrs.names, > > > >> > > > > > ipv6_addrs.n, > > > >> > > > > > > + sb_addr_set_v6); > > > >> > > > > > > + > > > >> > > > > > > + free(ipv4_addrs_name); > > > >> > > > > > > + free(ipv6_addrs_name); > > > >> > > > > > > + svec_destroy(&ipv4_addrs); > > > >> > > > > > > + svec_destroy(&ipv6_addrs); > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + return true; > > > >> > > > > > > +} > > > >> > > > > > > + > > > >> > > > > > > /* static functions. */ > > > >> > > > > > > static void > > > >> > > > > > > sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const > char > > > >> *name, > > > >> > > > > > > @@ -106,10 +206,11 @@ sync_address_set(struct > ovsdb_idl_txn > > > >> > > *ovnsb_txn, > > > >> > > > > > const char *name, > > > >> > > > > > > if (!sb_address_set) { > > > >> > > > > > > sb_address_set = > sbrec_address_set_insert(ovnsb_txn); > > > >> > > > > > > sbrec_address_set_set_name(sb_address_set, > name); > > > >> > > > > > > + sbrec_address_set_set_addresses(sb_address_set, > > > >> > > > > > > + addrs, n_addrs); > > > >> > > > > > > + } else { > > > >> > > > > > > + update_sb_addr_set(addrs, n_addrs, > sb_address_set); > > > >> > > > > > > } > > > >> > > > > > > - > > > >> > > > > > > - sbrec_address_set_set_addresses(sb_address_set, > > > >> > > > > > > - addrs, n_addrs); > > > >> > > > > > > } > > > >> > > > > > > > > > >> > > > > > > /* OVN_Southbound Address_Set table contains same > records as in > > > >> > > north > > > >> > > > > > > @@ -148,18 +249,7 @@ sync_address_sets( > > > >> > > > > > > > nb_port_group_table) { > > > >> > > > > > > struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER; > > > >> > > > > > > struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER; > > > >> > > > > > > - for (size_t i = 0; i < nb_port_group->n_ports; > i++) { > > > >> > > > > > > - for (size_t j = 0; j < > > > >> > > nb_port_group->ports[i]->n_addresses; > > > >> > > > > > j++) { > > > >> > > > > > > - const char *addrs = > > > >> > > > > > nb_port_group->ports[i]->addresses[j]; > > > >> > > > > > > - if (!is_dynamic_lsp_address(addrs)) { > > > >> > > > > > > - split_addresses(addrs, &ipv4_addrs, > > > >> > > &ipv6_addrs); > > > >> > > > > > > - } > > > >> > > > > > > - } > > > >> > > > > > > - if > (nb_port_group->ports[i]->dynamic_addresses) { > > > >> > > > > > > - > > > >> > > > > > > split_addresses(nb_port_group->ports[i]->dynamic_addresses, > > > >> > > > > > > - &ipv4_addrs, > &ipv6_addrs); > > > >> > > > > > > - } > > > >> > > > > > > - } > > > >> > > > > > > + build_port_group_address_set(nb_port_group, > > > >> &ipv4_addrs, > > > >> > > > > > &ipv6_addrs); > > > >> > > > > > > char *ipv4_addrs_name = xasprintf("%s_ip4", > > > >> > > nb_port_group->name); > > > >> > > > > > > char *ipv6_addrs_name = xasprintf("%s_ip6", > > > >> > > nb_port_group->name); > > > >> > > > > > > sync_address_set(ovnsb_txn, ipv4_addrs_name, > > > >> > > > > > > @@ -228,3 +318,85 @@ sync_address_sets( > > > >> > > > > > > } > > > >> > > > > > > shash_destroy(&sb_address_sets); > > > >> > > > > > > } > > > >> > > > > > > + > > > >> > > > > > > +static void > > > >> > > > > > > +update_sb_addr_set(const char **nb_addresses, size_t > > > >> n_addresses, > > > >> > > > > > > + const struct sbrec_address_set > *sb_as) > > > >> > > > > > > +{ > > > >> > > > > > > + struct expr_constant_set *cs_nb_as = > > > >> > > > > > > + expr_constant_set_create_integers( > > > >> > > > > > > + (const char *const *) nb_addresses, > n_addresses); > > > >> > > > > > > + struct expr_constant_set *cs_sb_as = > > > >> > > > > > > + expr_constant_set_create_integers( > > > >> > > > > > > + (const char *const *) sb_as->addresses, > > > >> > > sb_as->n_addresses); > > > >> > > > > > > + > > > >> > > > > > > + struct expr_constant_set *addr_added = NULL; > > > >> > > > > > > + struct expr_constant_set *addr_deleted = NULL; > > > >> > > > > > > + expr_constant_set_integers_diff(cs_sb_as, cs_nb_as, > > > >> > > &addr_added, > > > >> > > > > > > + &addr_deleted); > > > >> > > > > > > + > > > >> > > > > > > + struct ds ds = DS_EMPTY_INITIALIZER; > > > >> > > > > > > + if (addr_added && addr_added->n_values) { > > > >> > > > > > > + for (size_t i = 0; i < addr_added->n_values; > i++) { > > > >> > > > > > > + ds_clear(&ds); > > > >> > > > > > > + expr_constant_format(&addr_added->values[i], > > > >> > > EXPR_C_INTEGER, > > > >> > > > > > &ds); > > > >> > > > > > > + > sbrec_address_set_update_addresses_addvalue(sb_as, > > > >> > > > > > ds_cstr(&ds)); > > > >> > > > > > > + } > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + if (addr_deleted && addr_deleted->n_values) { > > > >> > > > > > > + for (size_t i = 0; i < addr_deleted->n_values; > i++) { > > > >> > > > > > > + ds_clear(&ds); > > > >> > > > > > > + > expr_constant_format(&addr_deleted->values[i], > > > >> > > > > > > + EXPR_C_INTEGER, &ds); > > > >> > > > > > > + > sbrec_address_set_update_addresses_delvalue(sb_as, > > > >> > > > > > ds_cstr(&ds)); > > > >> > > > > > > + } > > > >> > > > > > > + } > > > >> > > > > > > + > > > >> > > > > > > + ds_destroy(&ds); > > > >> > > > > > > + expr_constant_set_destroy(cs_nb_as); > > > >> > > > > > > + free(cs_nb_as); > > > >> > > > > > > + expr_constant_set_destroy(cs_sb_as); > > > >> > > > > > > + free(cs_sb_as); > > > >> > > > > > > + expr_constant_set_destroy(addr_added); > > > >> > > > > > > + free(addr_added); > > > >> > > > > > > + expr_constant_set_destroy(addr_deleted); > > > >> > > > > > > + free(addr_deleted); > > > >> > > > > > > +} > > > >> > > > > > > + > > > >> > > > > > > +static void > > > >> > > > > > > +build_port_group_address_set(const struct > nbrec_port_group > > > >> > > > > > *nb_port_group, > > > >> > > > > > > + struct svec *ipv4_addrs, > > > >> > > > > > > + struct svec *ipv6_addrs) > > > >> > > > > > > +{ > > > >> > > > > > > + for (size_t i = 0; i < nb_port_group->n_ports; i++) > { > > > >> > > > > > > + for (size_t j = 0; j < > > > >> > > nb_port_group->ports[i]->n_addresses; > > > >> > > > > > j++) { > > > >> > > > > > > + const char *addrs = > > > >> > > nb_port_group->ports[i]->addresses[j]; > > > >> > > > > > > + if (!is_dynamic_lsp_address(addrs)) { > > > >> > > > > > > + split_addresses(addrs, ipv4_addrs, > ipv6_addrs); > > > >> > > > > > > + } > > > >> > > > > > > + } > > > >> > > > > > > + if (nb_port_group->ports[i]->dynamic_addresses) > { > > > >> > > > > > > + > > > >> > > split_addresses(nb_port_group->ports[i]->dynamic_addresses, > > > >> > > > > > > + ipv4_addrs, ipv6_addrs); > > > >> > > > > > > + } > > > >> > > > > > > + } > > > >> > > > > > > +} > > > >> > > > > > > + > > > >> > > > > > > +/* Finds and returns the address set with the given > 'name', or > > > >> > > NULL if > > > >> > > > > > no such > > > >> > > > > > > + * address set exists. */ > > > >> > > > > > > +static const struct sbrec_address_set * > > > >> > > > > > > +sb_address_set_lookup_by_name(struct ovsdb_idl_index > > > >> > > > > > *sbrec_addr_set_by_name, > > > >> > > > > > > + const char *name) > > > >> > > > > > > +{ > > > >> > > > > > > + struct sbrec_address_set *target = > > > >> > > sbrec_address_set_index_init_row( > > > >> > > > > > > + sbrec_addr_set_by_name); > > > >> > > > > > > + sbrec_address_set_index_set_name(target, name); > > > >> > > > > > > + > > > >> > > > > > > + struct sbrec_address_set *retval = > > > >> > > sbrec_address_set_index_find( > > > >> > > > > > > + sbrec_addr_set_by_name, target); > > > >> > > > > > > + > > > >> > > > > > > + sbrec_address_set_index_destroy_row(target); > > > >> > > > > > > + > > > >> > > > > > > + return retval; > > > >> > > > > > > +} > > > >> > > > > > > diff --git a/northd/en-sb-sync.h b/northd/en-sb-sync.h > > > >> > > > > > > index f99d6a9fc..a63453fe5 100644 > > > >> > > > > > > --- a/northd/en-sb-sync.h > > > >> > > > > > > +++ b/northd/en-sb-sync.h > > > >> > > > > > > @@ -3,12 +3,18 @@ > > > >> > > > > > > > > > >> > > > > > > #include "lib/inc-proc-eng.h" > > > >> > > > > > > > > > >> > > > > > > +/* en_sb_sync engine node functions. */ > > > >> > > > > > > void *en_sb_sync_init(struct engine_node *, struct > engine_arg > > > >> *); > > > >> > > > > > > void en_sb_sync_run(struct engine_node *, void *data); > > > >> > > > > > > void en_sb_sync_cleanup(void *data); > > > >> > > > > > > > > > >> > > > > > > +/* en_address_set_sync engine node functions. */ > > > >> > > > > > > void *en_address_set_sync_init(struct engine_node *, > struct > > > >> > > engine_arg > > > >> > > > > > *); > > > >> > > > > > > void en_address_set_sync_run(struct engine_node *, void > *data); > > > >> > > > > > > void en_address_set_sync_cleanup(void *data); > > > >> > > > > > > +bool address_set_sync_nb_address_set_handler(struct > > > >> engine_node *, > > > >> > > > > > > + void > *data); > > > >> > > > > > > +bool address_set_sync_nb_port_group_handler(struct > engine_node > > > >> *, > > > >> > > > > > > + void *data); > > > >> > > > > > > > > > >> > > > > > > #endif > > > >> > > > > > > diff --git a/northd/inc-proc-northd.c > b/northd/inc-proc-northd.c > > > >> > > > > > > index b48f53f17..e2c25046a 100644 > > > >> > > > > > > --- a/northd/inc-proc-northd.c > > > >> > > > > > > +++ b/northd/inc-proc-northd.c > > > >> > > > > > > @@ -238,8 +238,10 @@ void inc_proc_northd_init(struct > > > >> > > ovsdb_idl_loop *nb, > > > >> > > > > > > * the NB database tables. > > > >> > > > > > > * Right now this engine only syncs the SB > Address_Set > > > >> table. > > > >> > > > > > > */ > > > >> > > > > > > - engine_add_input(&en_address_set_sync, > &en_nb_address_set, > > > >> > > NULL); > > > >> > > > > > > - engine_add_input(&en_address_set_sync, > &en_nb_port_group, > > > >> > > NULL); > > > >> > > > > > > + engine_add_input(&en_address_set_sync, > &en_nb_address_set, > > > >> > > > > > > + > address_set_sync_nb_address_set_handler); > > > >> > > > > > > + engine_add_input(&en_address_set_sync, > &en_nb_port_group, > > > >> > > > > > > + > address_set_sync_nb_port_group_handler); > > > >> > > > > > > engine_add_input(&en_address_set_sync, > > > >> &en_nb_load_balancer, > > > >> > > NULL); > > > >> > > > > > > engine_add_input(&en_address_set_sync, > > > >> > > &en_nb_load_balancer_group, > > > >> > > > > > NULL); > > > >> > > > > > > engine_add_input(&en_address_set_sync, > > > >> &en_nb_logical_router, > > > >> > > NULL); > > > >> > > > > > > @@ -248,8 +250,12 @@ void inc_proc_northd_init(struct > > > >> > > ovsdb_idl_loop *nb, > > > >> > > > > > > > > > >> > > > > > > /* We need the en_northd generated data as input to > > > >> > > > > > en_address_set_sync > > > >> > > > > > > * node to access the data generated by it (eg. > struct > > > >> > > ovn_datapath). > > > >> > > > > > > + * The handler is noop since en_northd always falls > back to > > > >> > > full > > > >> > > > > > recompute > > > >> > > > > > > + * (since it has no input handlers) and it doesn't > yet > > > >> > > indicate what > > > >> > > > > > > + * changed. It doesn't make sense to add NULL > handler for > > > >> this > > > >> > > input, > > > >> > > > > > > + * otherwise 'en_address_set_sync' will always fall > back to > > > >> > > full > > > >> > > > > > recompute. > > > >> > > > > > > */ > > > >> > > > > > > - engine_add_input(&en_address_set_sync, &en_northd, > NULL); > > > >> > > > > > > + engine_add_input(&en_address_set_sync, &en_northd, > > > >> > > > > > engine_noop_handler); > > > >> > > > > > > > > >> > > > > > I don't think we should use noop handler here. > en_address_set_sync > > > >> > > clearly > > > >> > > > > > depends on en_northd. So if en_northd changes (e.g. > ovn_datapath > > > >> > > changes), > > > >> > > > > > we do want the en_address_set_sync node to get recomputed. > > > >> > > > > > And I understand that the purpose of this patch is to make > sure > > > >> when > > > >> > > > > > address_set changes, we don't want to trigger any > recompute. > > > >> > > > > > > > >> > > > > Not really. If there are any NB address set changes, it > would > > > >> still > > > >> > > > > result in recompute of en_northd engine node and "en_lflow" > engine > > > >> > > > > nodes. > > > >> > > > > > > > >> > > > > This patch attempts to not recompute the NB to SB sync of > address > > > >> sets. > > > >> > > > > > >> > > So, do you mean we will expect that the en_northd node always > performans > > > >> > > recompute while in the same iterations the en_address_set_sync > node > > > >> > > performs I-P? This may be fine, > > > >> > That's correct. > > > >> > > > > >> > but I see a much bigger value if certain > > > >> > > changes don't trigger en_northd recompute at all. For example, > if there > > > >> are > > > >> > > only NB Address_Set changes, en_northd doesn't need to be > triggered, > > > >> right? > > > >> > > I am sure en_northd doesn't depend on NB address_set any more > (after > > > >> your > > > >> > > change). Simply removing the dependency "en_nb_address_set -> > en_northd" > > > >> > > would achieve that. I didn't check for NB port_group yet, > probably that > > > >> is > > > >> > > applicable, too. And then we don't need the noop handler at all > while > > > >> > > achieving an even better outcome. > > > >> > > > > >> > I definitely agree that we should try to improve so that en_northd > > > >> > recompute for many scenarios > > > >> > and I definitely want to put some effort into achieving this. > > > >> > > > > >> > That's a good suggestion. If en_northd doesn't depend on NB > Address > > > >> > Sets and NB port groups I'll remove it. > > > >> > > > > >> > > > > > >> > > > > > > > >> > > > > > > > >> > > > > In that case, > > > >> > > > > > we should remove the dependency between en_nb_address_set > -> > > > >> > > en_northd (if > > > >> > > > > > en_northd doesn't really depend on NB address_set). For > the other > > > >> > > > > > dependencies such as NB port_group, we should examine them > the > > > >> same > > > >> > > way: > > > >> > > > > > try to remove the dependency for en_northd, or implement > I-P > > > >> handler > > > >> > > in > > > >> > > > > > en_northd for those inputs, and make sure an input can be > I-P > > > >> handled > > > >> > > by > > > >> > > > > > all nodes depending on it. Otherwise, recompute is anyway > needed. > > > >> > > > > > In the current implementation, if I read the code > correctly, it > > > >> still > > > >> > > > > > triggers recompute even with this noop_handler, because > en_northd > > > >> > > depends > > > >> > > > > > on those inputs and it doesn't implement any I-P handler. > > > >> > > > > > > > >> > > > > Syncing SB address sets depends on 3 main inputs > > > >> > > > > > > > >> > > > > 1 . NB Address sets -> To sync NB address sets to SB address > sets. > > > >> > > > > This doesn't depend on the en_northd engine data. > > > >> > > > > > > > >> > > > > 2. NB Port groups. To sync the SB Address sets which > correspond to > > > >> > > > > NB Port groups (which northd generates). > > > >> > > > > This doesn't depend on the "en_northd" engine data. > > > >> > > > > > > > >> > > > > 3. NB Logical router / NB Logical load balancer / NB load > balancer > > > >> > > > > group -> To sync the SB address sets generated for the > routable load > > > >> > > > > balancer VIPs linked to a logical router. > > > >> > > > > This does depend on the "datapaths" hmap of en_northd > engine > > > >> data. > > > >> > > > > > > > >> > > > > > > >> > > > The reason I added a noop handler for en_northd input is > because of > > > >> > > > the above 3 conditions > > > >> > > > to sync SB address sets, > > > >> > > > > > > >> > > > The patch series adds the below inputs > > > >> > > > > > > >> > > > engine_add_input(&en_address_set_sync, &en_nb_address_set, > > > >> > > > address_set_sync_nb_address_set_handler); > > > >> > > > engine_add_input(&en_address_set_sync, &en_nb_port_group, > > > >> > > > address_set_sync_nb_port_group_handler); > > > >> > > > engine_add_input(&en_address_set_sync, &en_nb_load_balancer, > NULL); > > > >> > > > engine_add_input(&en_address_set_sync, > &en_nb_load_balancer_group, > > > >> NULL); > > > >> > > > engine_add_input(&en_address_set_sync, &en_nb_logical_router, > NULL); > > > >> > > > engine_add_input(&en_address_set_sync, &en_sb_address_set, > > > >> > > > engine_noop_handler); > > > >> > > > engine_add_input(&en_address_set_sync, &en_northd, > > > >> engine_noop_handler); > > > >> > > > > > > >> > > > > > > >> > > > Since (3) is handled by en_nb_load_balancer, > en_nb_load_balancer_group > > > >> > > > and en_nb_logical_router inputs, I thought it is fine > > > >> > > > to have a noop handler for en_northd and we add en_northd to > only > > > >> > > > access the 'datapaths' data of en_northd engine. > > > >> > > > > > > >> > > > > > >> > > Are you saying if "datapaths" changes, it means at least one of > the > > > >> other 3 > > > >> > > inputs (en_nb_load_balancer, en_nb_load_balancer_group and > > > >> > > en_nb_logical_router inputs) would have changed, too? > > > >> > > > > >> > Not exactly. > > > >> > > > >> If en_northd->datapaths changed, but en_nb_load_balancer, > > > >> en_nb_load_balancer_group and > > > >> en_nb_logical_router inputs didn't change, then the > en_address_set_sync > > > >> would not recompute, which would miss the handling of the > > > >> en_northd->datapaths change. > > > > > > > > > > > > > > > > (Copying the 3 main inputs for SB address set sync from the previous > reply) > > > > > > > > Syncing SB address sets depends on 3 main inputs > > > > > > > > 1 . NB Address sets -> To sync NB address sets to SB address sets. > > > > This doesn't depend on the en_northd engine data. > > > > > > > > 2. NB Port groups. To sync the SB Address sets which correspond to > > > > NB Port groups (which northd generates). > > > > This doesn't depend on the "en_northd" engine data. > > > > > > > > 3. NB Logical router / NB Logical load balancer / NB load balancer > > > > group -> To sync the SB address sets generated for the routable load > > > > balancer VIPs linked to a logical router. > > > > This does depend on the "datapaths" hmap of en_northd engine data. > > > > This specifically depends on "od->lb_ips->ips_v4_reachable and > od->lb_ips->ips_v6_reachable" > > > > > > > > > > > > So what's the issue if 'en_address_set_sync' miss handling of the > en_northd->datapaths changes if > > > > NB Logical router / NB Logical load balancer / NB load balancer > > > > group didn't change ? As there is no need to sync SB address sets ? > > > > > > Sorry If I confused you with the questions ? > > > > > > If en_northd->datapaths change but if NB Logical router / NB Logical > > > load balancer / NB load balancer group doesn't change, > > > it means there is no need to do any SB address set sync for the > > > address sets related to logical router reachable VIPs. > > > > > > I fail to understand your concerns ? What do you think the > > > 'en_address_set_sync' engine should do for the scenario you mentioned > > > ? > > > > > I was looking at it purely from the interface/dependency point of view: > > In the recompute function en_address_set_sync_run()->sync_address_sets() > it does take en_northd->datapaths as an input. So, without examining every > line of the function, I would assume that if datapaths changes, the output > (SB address_set) would possibly change, and the related data needs to be > re-handled. If that handling is missed, it is a potential I-P bug. > > However, if we know that en_northd->datapaths is somehow linked to the > other inputs of the en_address_set_sync that would trigger a recompute of > this node, then of course we can ignore the change of datapaths with a noop > handler. > > This is how we would normally reason about the correctness of the I-P. > > > > But you also said that the above statement "en_northd->datapaths is > somehow linked to the other inputs of the en_address_set_sync that would > trigger a recompute of this node" is "not exactly" true, yet you claimed > that the datapath input change doesn't need to be handled. > > By looking into more details, I can tell (not very confidently) that the > above statement is not true because it is possible that a LS datapath > changes while en_nb_load_balancer, en_nb_load_balancer_group and > en_nb_logical_router doesn't change, which is ok because in the > en_address_set_sync we only care about LR datapath changes. > > So, more precisely: > > 1. en_address_set_sync node takes datapaths as an input, but it only > cares about the LR datapath changes > > 2. if a LR datapath changes, it means there must be a change in > en_nb_logical_router, which is already an input to the en_address_set_sync > node with a NULL handler, to trigger a recompute in such cases. > > If the above 2 statements are true, we can say the noop handler can be > used without impacting correctness. Are they true? If so, we need to put > these in the comments, because these details are not obvious in the > dependency/interfaces. > > In fact I am not very confident about 2). I didn't examine how the each > fields of the datapath structure is populated. It is not an easy task to > tell that. Even if we can tell that for now, it is hard to ensure future > changes wouldn't break that without noticing in code reviews. This is why I > am realy reluctant to use noop handler. > > > > In general, trying to incrementally compute a node that depends on a node > is recomputed (without change tracking) is going to be really difficult (or > error prone). > > > > I suggest that let's avoid the noop handler and implement end-to-end I-P > for the nb_address_set and nb_port_group changes. We can take a two-step > approach. > Step1: I-P for nb_address_set changes. This means simply remove the > en_northd <- en_nb_address_set dependency on top of this current patch. > nb_address_set changes are handled incrementally without triggering > recompute in en_northd.
The present patch series does the SB address set sync by considering the 3 main inputs from the NB db i.e changes to NB Address Sets, NB Port groups and NB Logical router/load balancer - kind of you can say it is taking a bottom up approach. From your suggestions, I understand this way. Please correct me if I'm wrong. 1. Take the top down approach. 2. When the NB address set changes, just sync the SB address sets which directly map to this. 3. When NB port group changes sync the SB port groups and also sync Your suggestion is > Step2: I-P for nb_port_group changes. This can be done with follow up > patches. There is more work but still looks straightforward. We need a > en_ovn_port_groups node that maintains the struct ovn_port_group data, and > also split a en_ovn_ports node from en_northd as the input of the > en_ovn_port_groups. The dependency would be: > en_ovn_ports <- ... (the NB/SB tables required to compute the > build_ports()) > en_ovn_port_groups <- en_nb_port_group (need I-P handler) > en_ovn_port_groups <- en_ovn_ports > en_northd <- en_ovn_port_groups (need I-P handler) > The only parts that need I-P handlers are en_ovn_port_groups <- > en_nb_port_group and en_northd <- en_ovn_port_groups, and the rest are > mostly refactoring engine nodes and functions. > > This way, both nb_address_set and nb_port_group are handled incrementally > end to end, which is a better overall performance gain of ovn-northd > (probably not that obvious for OVN IC, but would be significant for regular > OVN). As a side-product, no need for noop handlers. Does this make sense? > > Thanks, > Han > > > Thanks, > > Han > > > > > > > > Thanks > > > Numan > > > > > > > > > > group > > > > > > > > Thanks > > > > Numan > > > > > > > > > > > >> > > > >> > > > > >> > Lets say either or all of the above 3 (i.e NB load balancer, NB > load > > > >> > balancer group, NB Logical Router) > > > >> > change in the iteration, then since there is no handler for these > > > >> > inputs in en_address_set_sync node, a full recompute is triggered. > > > >> > Also en_northd would have changed too (and updated the "datapaths" > > > >> > hmap of en_northd data). A full recompute ensures that > > > >> > the SB address sets are synced correctly. > > > >> > > > > >> > However if none of the above 3 change but something else changes > (for > > > >> > example NB logical switch port) then the en_northd engine would > have > > > >> > changed. > > > >> > But there is no need to sync the SB address sets and noop handler > for > > > >> > en_northd input would be ok. > > > >> > > > > >> > > > >> What if that "something else" is en_northd->datapaths? > > > >> > > > >> Thanks, > > > >> Han > > > >> > > > >> > Let me know if this makes sense to you. If so, I'll add proper > > > >> > documentation with noop handler for en_northd input. > > > >> > > > > >> > Otherwise I'll add a handler for en_northd input and iterate > through > > > >> > the data paths and sync the address sets for the logical router > > > >> > reachable VIPS. > > > >> > > > > >> > > If that's the case, I agree it is correct, but the documentation > for > > > >> this > > > >> > > noop handler should call it out clearly. And in the future if > this > > > >> > > statement is not true any more, the noop handler must be removed > at the > > > >> > > same time, too. I'd avoid such pains if possible. > > > >> > > > > >> > Agree. I'll add more documentation in v4. > > > >> > > > > >> > Thanks for the reviews. > > > >> > > > > >> > Numan > > > >> > > > > >> > > > > > >> > > Thanks, > > > >> > > Han > > > >> > > > > > >> > > > I do remember we had similar conversations earlier with > > > >> > > > en_runtime_data input for en_lflow_output_handler. > > > >> > > > > > > >> > > > I still think it is ok to have a noop handler and to document > it > > > >> > > > (which this patch does). > > > >> > > > > > > >> > > > Thanks > > > >> > > > Numan > > > >> > > > > > > >> > > > > > > > >> > > > > In v4, I'll add the handler > (address_set_sync_northd_handler) for > > > >> > > > > en_northd engine input to the 'en_address_set_sync' node. > Since > > > >> > > > > en_northd engine doesn't have handlers for its input nodes, > it is > > > >> not > > > >> > > > > possible to determine > > > >> > > > > what changed. So the handler > address_set_sync_northd_handler() will > > > >> > > > > loop through all the datapaths and sync the SB address sets > for > > > >> > > > > router reachable vips. > > > >> > > > > > > > >> > > > > Whenever the en_northd engine is updated (which is the case > for any > > > >> NB > > > >> > > > > db change), this handler will be invoked (even though there > is no > > > >> real > > > >> > > > > need to sync the SB address sets). But I think that is ok. > > > >> > > > > > > > >> > > > > Please let me know what you think. > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > > > >> > > > > > > engine_add_input(&en_sb_sync, &en_address_set_sync, > NULL); > > > >> > > > > > > engine_add_input(&en_northd_output, &en_sb_sync, > > > >> > > > > > > @@ -300,6 +306,12 @@ void inc_proc_northd_init(struct > > > >> > > ovsdb_idl_loop *nb, > > > >> > > > > > > engine_ovsdb_node_add_index(&en_sb_mac_binding, > > > >> > > > > > > > > > >> "sbrec_mac_binding_by_datapath", > > > >> > > > > > > > sbrec_mac_binding_by_datapath); > > > >> > > > > > > + > > > >> > > > > > > + struct ovsdb_idl_index *sbrec_address_set_by_name > > > >> > > > > > > + = ovsdb_idl_index_create1(sb->idl, > > > >> > > &sbrec_address_set_col_name); > > > >> > > > > > > + engine_ovsdb_node_add_index(&en_sb_address_set, > > > >> > > > > > > + > "sbrec_address_set_by_name", > > > >> > > > > > > + > sbrec_address_set_by_name); > > > >> > > > > > > } > > > >> > > > > > > > > > >> > > > > > > void inc_proc_northd_run(struct ovsdb_idl_txn > *ovnnb_txn, > > > >> > > > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > >> > > > > > > index e849afd85..06de90c80 100644 > > > >> > > > > > > --- a/tests/ovn-northd.at > > > >> > > > > > > +++ b/tests/ovn-northd.at > > > >> > > > > > > @@ -7929,3 +7929,55 @@ AT_CHECK([grep "lr_in_arp_resolve" > > > >> R1flows | > > > >> > > grep > > > >> > > > > > priority=90 | sort], [0], [dnl > > > >> > > > > > > > > > >> > > > > > > AT_CLEANUP > > > >> > > > > > > ]) > > > >> > > > > > > + > > > >> > > > > > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > > >> > > > > > > +AT_SETUP([Address set incremental processing]) > > > >> > > > > > > > > >> > > > > > The test case seems only checking if it is using mutation, > but > > > >> doesn't > > > >> > > > > > check if ovn-northd did recompute or I-P. I think it > should check > > > >> > > whether > > > >> > > > > > it has performed recompute, probably using > inc-engine/show-stats. > > > >> > > > > > > > >> > > > > Ack. > > > >> > > > > > > > >> > > > > Thanks > > > >> > > > > Numan > > > >> > > > > > > > >> > > > > > > > > >> > > > > > Thanks, > > > >> > > > > > Han > > > >> > > > > > > > > >> > > > > > > +ovn_start > > > >> > > > > > > + > > > >> > > > > > > +foo_as_uuid=$(ovn-nbctl create address_set name=foo > > > >> > > > > > addresses=\"1.1.1.1\",\"1.1.1.2\") > > > >> > > > > > > +ovn-nbctl --wait=sb sync > > > >> > > > > > > + > > > >> > > > > > > +check_column '1.1.1.1 1.1.1.2' Address_Set addresses > name=foo > > > >> > > > > > > + > > > >> > > > > > > +rm -f northd/ovn-northd.log > > > >> > > > > > > +check as northd ovn-appctl -t NORTHD_TYPE vlog/reopen > > > >> > > > > > > +check as northd ovn-appctl -t NORTHD_TYPE vlog/set > jsonrpc:dbg > > > >> > > > > > > + > > > >> > > > > > > +check ovn-nbctl --wait=sb add address_set $foo_as_uuid > > > >> addresses > > > >> > > 1.1.1.3 > > > >> > > > > > > +check_column '1.1.1.1 1.1.1.2 1.1.1.3' Address_Set > addresses > > > >> > > name=foo > > > >> > > > > > > + > > > >> > > > > > > +AT_CHECK([grep transact northd/ovn-northd.log | grep > > > >> Address_Set | > > > >> > > \ > > > >> > > > > > > +grep -c mutate], [0], [1 > > > >> > > > > > > +]) > > > >> > > > > > > + > > > >> > > > > > > +check ovn-nbctl --wait=sb add address_set $foo_as_uuid > > > >> addresses \ > > > >> > > > > > > +1.1.1.4 -- remove address_set $foo_as_uuid addresses > 1.1.1.1 > > > >> > > > > > > +check_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set > addresses > > > >> > > name=foo > > > >> > > > > > > + > > > >> > > > > > > +AT_CHECK([grep transact northd/ovn-northd.log | grep > > > >> Address_Set | > > > >> > > \ > > > >> > > > > > > +grep -c mutate], [0], [2 > > > >> > > > > > > +]) > > > >> > > > > > > + > > > >> > > > > > > +# Pause ovn-northd and add/remove few addresses. when > it is > > > >> > > resumed > > > >> > > > > > > +# it should use mutate for updating the address sets. > > > >> > > > > > > +check as northd ovn-appctl -t NORTHD_TYPE pause > > > >> > > > > > > +check as northd-backup ovn-appctl -t NORTHD_TYPE pause > > > >> > > > > > > + > > > >> > > > > > > +check ovn-nbctl add address_set $foo_as_uuid addresses > 1.1.1.5 > > > >> > > > > > > +check ovn-nbctl add address_set $foo_as_uuid addresses > 1.1.1.6 > > > >> > > > > > > +check ovn-nbctl remove address_set $foo_as_uuid > addresses > > > >> 1.1.1.2 > > > >> > > > > > > + > > > >> > > > > > > +check_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set > addresses > > > >> > > name=foo > > > >> > > > > > > + > > > >> > > > > > > +# Resume northd now > > > >> > > > > > > +check as northd ovn-appctl -t NORTHD_TYPE resume > > > >> > > > > > > +check ovn-nbctl --wait=sb sync > > > >> > > > > > > + > > > >> > > > > > > +check_column '1.1.1.3 1.1.1.4 1.1.1.5 1.1.1.6' > Address_Set > > > >> > > addresses > > > >> > > > > > name=foo > > > >> > > > > > > + > > > >> > > > > > > +AT_CHECK([grep transact northd/ovn-northd.log | grep > > > >> Address_Set | > > > >> > > \ > > > >> > > > > > > +grep -c mutate], [0], [3 > > > >> > > > > > > +]) > > > >> > > > > > > + > > > >> > > > > > > +AT_CLEANUP > > > >> > > > > > > +]) > > > >> > > > > > > -- > > > >> > > > > > > 2.38.1 > > > >> > > > > > > > > > >> > > > > > > _______________________________________________ > > > >> > > > > > > dev mailing list > > > >> > > > > > > [email protected] > > > >> > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >> > > > > > _______________________________________________ > > > >> > > > > > dev mailing list > > > >> > > > > > [email protected] > > > >> > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >> > > > > > > > > >> > > _______________________________________________ > > > >> > > dev mailing list > > > >> > > [email protected] > > > >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > >> > > > > > >> _______________________________________________ > > > >> 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
