On Sat, Nov 19, 2022 at 8:02 PM Han Zhou <[email protected]> wrote: > > On Sat, Nov 19, 2022 at 3:35 PM Numan Siddique <[email protected]> wrote: > > > > On Sat, Nov 19, 2022 at 6:24 PM Numan Siddique <[email protected]> > wrote: > > > > > > 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? > > Yes. I'm quite certain about it. > > Thanks for confirming. > > > > 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. > > > > > > > > > > (Please ignore the previous reply. It was incomplete and accidentally > sent) > > > > 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 the below. 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 maps to this. > > 3. When NB port group changes > > - sync the SB port groups and also > > - sync the SB address sets generated for NB port groups. > > 4. When NB logical router / Load balancer changes, sync the SB > > address sets which are generated for logical router routable VIPs. > > > > Is this what you're trying to say ? > > > > If not, then I'm not clear in step 1 on how we would sync the SB > > address sets related to NB port groups and SB address sets related to > > logical router routable VIPs. > > > Yes, I think I-P engine by far is always top-down. If some node can't > handle a change incrementally, then the node's change is not tracked (or > hard to be tracked in an efficient way), so any nodes that depend on it > would have to recompute. Taking a top-down approach ensures every node in > the dependency path of the changed input handles the changes incrementally. > In your above summary, 2) is the step1 in my suggestion and 3) is the > step2. The point 4) is not mentioned in my suggested two steps, because > even this patch didn't consider incremental processing for those, so I > assume it is ok to leave them for now and always trigger recompute for > those inputs. > > > > > > > > > > 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 > > > > So does that mean the node - en_ovn_ports will contain "hmap" of > > "struct ovn_ports" ? > > > Yes. > > > I tend to agree with this approach in general. I still need to spend > > some time thinking of this. > > > Thanks Numan. I think this patch is almost ready for step1, just need some > minor changes: > 1. Delete nbrec_address_set_table from the struct northd_input, and related > engine node dependency definitions. I had a test and all cases passed. > 2. Remove the noop handler: engine_add_input(&en_address_set_sync, > &en_northd, NULL) > 3. Update the test case to check I-P stats counter for nb address_set > changes (make sure recompute didn't happen for the high-cost nodes: > en_northd and en_lflow, and if prefered, en_address_set_sync as well) >
Please take a look at v4. FYI - Looks like a few of ovn-k8s control plane tests are failing with this patch series. I need to investigate why. I submitted v4 nevertheless. > For step2, I agree more work is needed. Please let me know if I can help. Sure. I'll let you know. Thanks Numan > > Thanks, > Han > > > Thanks > > Numan > > > > > > > > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
