Message ID | 20230607063658.23975-1-amusil@redhat.com |
---|---|
State | Accepted |
Headers | show |
Series | [ovs-dev,v3] northd: Fix address set incremental processing | expand |
On Tue, Jun 6, 2023 at 11:37 PM Ales Musil <amusil@redhat.com> wrote: > > The incremental processing is broken for addresses > that have mask which could "erase" portion of the address > itself e.g. 10.16.0.47/4, after applying the mask with token > parser the address becomes 0.0.0.0/4, which is fine for > logical flows. However, for the deletion/addition to database > we need the original string representation which cannot be > built from the parsed token. > > Use strcmp over already sorted lists which should give > us the needed diff. The time complexity didn't change, > but the performance was slightly improved, see the numbers > below. The setup was created by the scale script from Han [0]. > > Numbers with expr: > Time spent on processing nb_cfg 1: > ovn-northd delay before processing: 21ms > ovn-northd completion: 544ms > ovn-controller(s) completion: 544ms > Time spent on processing nb_cfg 2: > ovn-northd delay before processing: 17ms > ovn-northd completion: 537ms > ovn-controller(s) completion: 535ms > Time spent on processing nb_cfg 3: > ovn-northd delay before processing: 20ms > ovn-northd completion: 552ms > ovn-controller(s) completion: 550ms > Time spent on processing nb_cfg 4: > ovn-northd delay before processing: 16ms > ovn-northd completion: 529ms > ovn-controller(s) completion: 528ms > > Numbers with the strcmp: > Time spent on processing nb_cfg 1: > ovn-northd delay before processing: 24ms > ovn-northd completion: 521ms > ovn-controller(s) completion: 521ms > Time spent on processing nb_cfg 2: > ovn-northd delay before processing: 17ms > ovn-northd completion: 500ms > ovn-controller(s) completion: 500ms > Time spent on processing nb_cfg 3: > ovn-northd delay before processing: 17ms > ovn-northd completion: 496ms > ovn-controller(s) completion: 497ms > Time spent on processing nb_cfg 4: > ovn-northd delay before processing: 18ms > ovn-northd completion: 502ms > ovn-controller(s) completion: 500ms > > [0] https://github.com/hzhou8/ovn-test-script/blob/e1149318201309db6d96ae8d5a33fcfbbe1872a3/test_ovn_controller_scale.sh > > Fixes: 0d5e0a6ced49 ("northd: Add I-P for syncing SB address sets.") > Reported-at: https://bugzilla.redhat.com/2196885 > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2023-May/052426.html > Reported-By: 张祖建 <zhangzujian.7@gmail.com> > Signed-off-by: Ales Musil <amusil@redhat.com> > --- > v2: Address comments from Ilya: > - Use "custom" algorithm to prevent a ton of allocations with svec. > - Add original reporter. > v3: Rebase on top of current main. > Address comment from Han: > - Add test case for the problematic address. > --- > northd/en-sync-sb.c | 183 +++++++++++++++++++++++++++----------------- > tests/ovn-northd.at | 10 ++- > 2 files changed, 118 insertions(+), 75 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 758f00bfd..87402d6fb 100644 > --- a/northd/en-sync-sb.c > +++ b/northd/en-sync-sb.c > @@ -22,7 +22,6 @@ > #include "openvswitch/util.h" > > #include "en-sync-sb.h" > -#include "include/ovn/expr.h" > #include "lib/inc-proc-eng.h" > #include "lib/lb.h" > #include "lib/ovn-nb-idl.h" > @@ -34,8 +33,15 @@ > > VLOG_DEFINE_THIS_MODULE(en_sync_to_sb); > > +/* This is just a type wrapper to enforce that it has to be sorted. */ > +struct sorted_addresses { > + const char **arr; > + size_t n; > +}; > + > + > static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > - const char **addrs, size_t n_addrs, > + struct sorted_addresses *addresses, > struct shash *sb_address_sets); > static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > const struct nbrec_address_set_table *, > @@ -44,11 +50,17 @@ static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > const struct ovn_datapaths *lr_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, > +static void update_sb_addr_set(struct sorted_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); > +static struct sorted_addresses > +sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as); > +static struct sorted_addresses > +sorted_addresses_from_svec(struct svec *addresses); > +static struct sorted_addresses > +sorted_addresses_from_sset(struct sset* addresses); > > void * > en_sync_to_sb_init(struct engine_node *node OVS_UNUSED, > @@ -133,8 +145,9 @@ sync_to_sb_addr_set_nb_address_set_handler(struct engine_node *node, > if (!sb_addr_set) { > return false; > } > - update_sb_addr_set((const char **) nb_addr_set->addresses, > - nb_addr_set->n_addresses, sb_addr_set); > + struct sorted_addresses addrs = > + sorted_addresses_from_nbrec(nb_addr_set); > + update_sb_addr_set(&addrs, sb_addr_set); > } > > return true; > @@ -180,10 +193,14 @@ sync_to_sb_addr_set_nb_port_group_handler(struct engine_node *node, > 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); > + > + struct sorted_addresses ipv4_addrs_sorted = > + sorted_addresses_from_svec(&ipv4_addrs); > + struct sorted_addresses ipv6_addrs_sorted = > + sorted_addresses_from_svec(&ipv6_addrs); > + > + update_sb_addr_set(&ipv4_addrs_sorted, sb_addr_set_v4); > + update_sb_addr_set(&ipv6_addrs_sorted, sb_addr_set_v6); > > free(ipv4_addrs_name); > free(ipv6_addrs_name); > @@ -197,7 +214,7 @@ sync_to_sb_addr_set_nb_port_group_handler(struct engine_node *node, > /* static functions. */ > static void > sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, > - const char **addrs, size_t n_addrs, > + struct sorted_addresses *addresses, > struct shash *sb_address_sets) > { > const struct sbrec_address_set *sb_address_set; > @@ -206,10 +223,10 @@ sync_addr_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); > + sbrec_address_set_set_addresses(sb_address_set, addresses->arr, > + addresses->n); > } else { > - update_sb_addr_set(addrs, n_addrs, sb_address_set); > + update_sb_addr_set(addresses, sb_address_set); > } > } > > @@ -244,8 +261,11 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > > /* Service monitor MAC. */ > const char *svc_monitor_macp = northd_get_svc_monitor_mac(); > - sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, > - &sb_address_sets); > + struct sorted_addresses svc = { > + .arr = &svc_monitor_macp, > + .n = 1, > + }; > + sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc, &sb_address_sets); > > /* sync port group generated address sets first */ > const struct nbrec_port_group *nb_port_group; > @@ -256,14 +276,16 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > 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); > + > + struct sorted_addresses ipv4_addrs_sorted = > + sorted_addresses_from_svec(&ipv4_addrs); > + struct sorted_addresses ipv6_addrs_sorted = > + sorted_addresses_from_svec(&ipv6_addrs); > + > sync_addr_set(ovnsb_txn, ipv4_addrs_name, > - /* "char **" is not compatible with "const char **" */ > - (const char **) ipv4_addrs.names, > - ipv4_addrs.n, &sb_address_sets); > + &ipv4_addrs_sorted, &sb_address_sets); > sync_addr_set(ovnsb_txn, ipv6_addrs_name, > - /* "char **" is not compatible with "const char **" */ > - (const char **) ipv6_addrs.names, > - ipv6_addrs.n, &sb_address_sets); > + &ipv6_addrs_sorted, &sb_address_sets); > free(ipv4_addrs_name); > free(ipv6_addrs_name); > svec_destroy(&ipv4_addrs); > @@ -278,27 +300,26 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > if (sset_count(&od->lb_ips->ips_v4_reachable)) { > char *ipv4_addrs_name = lr_lb_address_set_name(od->tunnel_key, > AF_INET); > - const char **ipv4_addrs = > - sset_array(&od->lb_ips->ips_v4_reachable); > > - sync_addr_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, > - sset_count(&od->lb_ips->ips_v4_reachable), > - &sb_address_sets); > + struct sorted_addresses ipv4_addrs_sorted = > + sorted_addresses_from_sset(&od->lb_ips->ips_v4_reachable); > + > + sync_addr_set(ovnsb_txn, ipv4_addrs_name, > + &ipv4_addrs_sorted, &sb_address_sets); > + free(ipv4_addrs_sorted.arr); > free(ipv4_addrs_name); > - free(ipv4_addrs); > } > > if (sset_count(&od->lb_ips->ips_v6_reachable)) { > char *ipv6_addrs_name = lr_lb_address_set_name(od->tunnel_key, > AF_INET6); > - const char **ipv6_addrs = > - sset_array(&od->lb_ips->ips_v6_reachable); > + struct sorted_addresses ipv6_addrs_sorted = > + sorted_addresses_from_sset(&od->lb_ips->ips_v6_reachable); > > - sync_addr_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, > - sset_count(&od->lb_ips->ips_v6_reachable), > - &sb_address_sets); > + sync_addr_set(ovnsb_txn, ipv6_addrs_name, > + &ipv6_addrs_sorted, &sb_address_sets); > + free(ipv6_addrs_sorted.arr); > free(ipv6_addrs_name); > - free(ipv6_addrs); > } > } > > @@ -307,10 +328,10 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > const struct nbrec_address_set *nb_address_set; > NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, > nb_address_set_table) { > + struct sorted_addresses addrs = > + sorted_addresses_from_nbrec(nb_address_set); > sync_addr_set(ovnsb_txn, nb_address_set->name, > - /* "char **" is not compatible with "const char **" */ > - (const char **) nb_address_set->addresses, > - nb_address_set->n_addresses, &sb_address_sets); > + &addrs, &sb_address_sets); > } > > struct shash_node *node; > @@ -322,48 +343,39 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, > } > > static void > -update_sb_addr_set(const char **nb_addresses, size_t n_addresses, > +update_sb_addr_set(struct sorted_addresses *nb_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)); > + size_t nb_index, sb_index; > + > + const char **nb_arr = nb_addresses->arr; > + char **sb_arr = sb_as->addresses; > + size_t nb_n = nb_addresses->n; > + size_t sb_n = sb_as->n_addresses; > + > + for (nb_index = sb_index = 0; nb_index < nb_n && sb_index < sb_n; ) { > + int cmp = strcmp(nb_arr[nb_index], sb_arr[sb_index]); > + if (cmp < 0) { > + sbrec_address_set_update_addresses_addvalue(sb_as, > + nb_arr[nb_index]); > + nb_index++; > + } else if (cmp > 0) { > + sbrec_address_set_update_addresses_delvalue(sb_as, > + sb_arr[sb_index]); > + sb_index++; > + } else { > + nb_index++; > + sb_index++; > } > } > > - 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)); > - } > + for (; nb_index < nb_n; nb_index++) { > + sbrec_address_set_update_addresses_addvalue(sb_as, nb_arr[nb_index]); > } > > - 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); > + for (; sb_index < sb_n; sb_index++) { > + sbrec_address_set_update_addresses_delvalue(sb_as, sb_arr[sb_index]); > + } > } > > static void > @@ -402,3 +414,32 @@ sb_address_set_lookup_by_name(struct ovsdb_idl_index *sbrec_addr_set_by_name, > > return retval; > } > + > +static struct sorted_addresses > +sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as) > +{ > + /* The DB is already sorted. */ > + return (struct sorted_addresses) { > + .arr = (const char **) nb_as->addresses, > + .n = nb_as->n_addresses, > + }; > +} > + > +static struct sorted_addresses > +sorted_addresses_from_svec(struct svec *addresses) > +{ > + svec_sort(addresses); > + return (struct sorted_addresses) { > + .arr = (const char **) addresses->names, > + .n = addresses->n, > + }; > +} > + > +static struct sorted_addresses > +sorted_addresses_from_sset(struct sset* addresses) > +{ > + return (struct sorted_addresses) { > + .arr = sset_sort(addresses), > + .n = sset_count(addresses), > + }; > +} > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 6736429ae..64bb9495a 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -8835,8 +8835,9 @@ 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 as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > -check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.3 > -wait_column '1.1.1.1 1.1.1.2 1.1.1.3' Address_Set addresses name=foo > +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.3 -- \ > + add address_set $foo_as_uuid addresses 1.1.2.1/4 > +wait_column '1.1.1.1 1.1.1.2 1.1.1.3 1.1.2.1/4' Address_Set addresses name=foo > > # There should be no recompute of the sync_to_sb_addr_set engine node . > AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_to_sb_addr_set recompute], [0], [0 > @@ -8846,8 +8847,9 @@ AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \ > grep -c mutate], [0], [1 > ]) > > -check ovn-nbctl add address_set $foo_as_uuid addresses \ > -1.1.1.4 -- remove address_set $foo_as_uuid addresses 1.1.1.1 > +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.4 -- \ > + remove address_set $foo_as_uuid addresses 1.1.1.1 -- \ > + remove address_set $foo_as_uuid addresses 1.1.2.1/4 > wait_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set addresses name=foo > > # There should be no recompute of the sync_to_sb_addr_set engine node . > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Ales. I applied with the below minor change that fixes the problem reported by checkpatch.py. Also backported to branch-23.06, 23.03, 22.12. Regards, Han ------------ 8>< --------------------------------------------------------------- ><8 --------------- diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 87402d6fbbcb..d7fea981f208 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -60,7 +60,7 @@ sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as); static struct sorted_addresses sorted_addresses_from_svec(struct svec *addresses); static struct sorted_addresses -sorted_addresses_from_sset(struct sset* addresses); +sorted_addresses_from_sset(struct sset *addresses); void * en_sync_to_sb_init(struct engine_node *node OVS_UNUSED, @@ -353,7 +353,7 @@ update_sb_addr_set(struct sorted_addresses *nb_addresses, size_t nb_n = nb_addresses->n; size_t sb_n = sb_as->n_addresses; - for (nb_index = sb_index = 0; nb_index < nb_n && sb_index < sb_n; ) { + for (nb_index = sb_index = 0; nb_index < nb_n && sb_index < sb_n;) { int cmp = strcmp(nb_arr[nb_index], sb_arr[sb_index]); if (cmp < 0) { sbrec_address_set_update_addresses_addvalue(sb_as, @@ -436,7 +436,7 @@ sorted_addresses_from_svec(struct svec *addresses) } static struct sorted_addresses -sorted_addresses_from_sset(struct sset* addresses) +sorted_addresses_from_sset(struct sset *addresses) { return (struct sorted_addresses) { .arr = sset_sort(addresses),
diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c index 758f00bfd..87402d6fb 100644 --- a/northd/en-sync-sb.c +++ b/northd/en-sync-sb.c @@ -22,7 +22,6 @@ #include "openvswitch/util.h" #include "en-sync-sb.h" -#include "include/ovn/expr.h" #include "lib/inc-proc-eng.h" #include "lib/lb.h" #include "lib/ovn-nb-idl.h" @@ -34,8 +33,15 @@ VLOG_DEFINE_THIS_MODULE(en_sync_to_sb); +/* This is just a type wrapper to enforce that it has to be sorted. */ +struct sorted_addresses { + const char **arr; + size_t n; +}; + + static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, - const char **addrs, size_t n_addrs, + struct sorted_addresses *addresses, struct shash *sb_address_sets); static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, const struct nbrec_address_set_table *, @@ -44,11 +50,17 @@ static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, const struct ovn_datapaths *lr_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, +static void update_sb_addr_set(struct sorted_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); +static struct sorted_addresses +sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as); +static struct sorted_addresses +sorted_addresses_from_svec(struct svec *addresses); +static struct sorted_addresses +sorted_addresses_from_sset(struct sset* addresses); void * en_sync_to_sb_init(struct engine_node *node OVS_UNUSED, @@ -133,8 +145,9 @@ sync_to_sb_addr_set_nb_address_set_handler(struct engine_node *node, if (!sb_addr_set) { return false; } - update_sb_addr_set((const char **) nb_addr_set->addresses, - nb_addr_set->n_addresses, sb_addr_set); + struct sorted_addresses addrs = + sorted_addresses_from_nbrec(nb_addr_set); + update_sb_addr_set(&addrs, sb_addr_set); } return true; @@ -180,10 +193,14 @@ sync_to_sb_addr_set_nb_port_group_handler(struct engine_node *node, 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); + + struct sorted_addresses ipv4_addrs_sorted = + sorted_addresses_from_svec(&ipv4_addrs); + struct sorted_addresses ipv6_addrs_sorted = + sorted_addresses_from_svec(&ipv6_addrs); + + update_sb_addr_set(&ipv4_addrs_sorted, sb_addr_set_v4); + update_sb_addr_set(&ipv6_addrs_sorted, sb_addr_set_v6); free(ipv4_addrs_name); free(ipv6_addrs_name); @@ -197,7 +214,7 @@ sync_to_sb_addr_set_nb_port_group_handler(struct engine_node *node, /* static functions. */ static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name, - const char **addrs, size_t n_addrs, + struct sorted_addresses *addresses, struct shash *sb_address_sets) { const struct sbrec_address_set *sb_address_set; @@ -206,10 +223,10 @@ sync_addr_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); + sbrec_address_set_set_addresses(sb_address_set, addresses->arr, + addresses->n); } else { - update_sb_addr_set(addrs, n_addrs, sb_address_set); + update_sb_addr_set(addresses, sb_address_set); } } @@ -244,8 +261,11 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, /* Service monitor MAC. */ const char *svc_monitor_macp = northd_get_svc_monitor_mac(); - sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1, - &sb_address_sets); + struct sorted_addresses svc = { + .arr = &svc_monitor_macp, + .n = 1, + }; + sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc, &sb_address_sets); /* sync port group generated address sets first */ const struct nbrec_port_group *nb_port_group; @@ -256,14 +276,16 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, 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); + + struct sorted_addresses ipv4_addrs_sorted = + sorted_addresses_from_svec(&ipv4_addrs); + struct sorted_addresses ipv6_addrs_sorted = + sorted_addresses_from_svec(&ipv6_addrs); + sync_addr_set(ovnsb_txn, ipv4_addrs_name, - /* "char **" is not compatible with "const char **" */ - (const char **) ipv4_addrs.names, - ipv4_addrs.n, &sb_address_sets); + &ipv4_addrs_sorted, &sb_address_sets); sync_addr_set(ovnsb_txn, ipv6_addrs_name, - /* "char **" is not compatible with "const char **" */ - (const char **) ipv6_addrs.names, - ipv6_addrs.n, &sb_address_sets); + &ipv6_addrs_sorted, &sb_address_sets); free(ipv4_addrs_name); free(ipv6_addrs_name); svec_destroy(&ipv4_addrs); @@ -278,27 +300,26 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, if (sset_count(&od->lb_ips->ips_v4_reachable)) { char *ipv4_addrs_name = lr_lb_address_set_name(od->tunnel_key, AF_INET); - const char **ipv4_addrs = - sset_array(&od->lb_ips->ips_v4_reachable); - sync_addr_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs, - sset_count(&od->lb_ips->ips_v4_reachable), - &sb_address_sets); + struct sorted_addresses ipv4_addrs_sorted = + sorted_addresses_from_sset(&od->lb_ips->ips_v4_reachable); + + sync_addr_set(ovnsb_txn, ipv4_addrs_name, + &ipv4_addrs_sorted, &sb_address_sets); + free(ipv4_addrs_sorted.arr); free(ipv4_addrs_name); - free(ipv4_addrs); } if (sset_count(&od->lb_ips->ips_v6_reachable)) { char *ipv6_addrs_name = lr_lb_address_set_name(od->tunnel_key, AF_INET6); - const char **ipv6_addrs = - sset_array(&od->lb_ips->ips_v6_reachable); + struct sorted_addresses ipv6_addrs_sorted = + sorted_addresses_from_sset(&od->lb_ips->ips_v6_reachable); - sync_addr_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs, - sset_count(&od->lb_ips->ips_v6_reachable), - &sb_address_sets); + sync_addr_set(ovnsb_txn, ipv6_addrs_name, + &ipv6_addrs_sorted, &sb_address_sets); + free(ipv6_addrs_sorted.arr); free(ipv6_addrs_name); - free(ipv6_addrs); } } @@ -307,10 +328,10 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, const struct nbrec_address_set *nb_address_set; NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set, nb_address_set_table) { + struct sorted_addresses addrs = + sorted_addresses_from_nbrec(nb_address_set); sync_addr_set(ovnsb_txn, nb_address_set->name, - /* "char **" is not compatible with "const char **" */ - (const char **) nb_address_set->addresses, - nb_address_set->n_addresses, &sb_address_sets); + &addrs, &sb_address_sets); } struct shash_node *node; @@ -322,48 +343,39 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn, } static void -update_sb_addr_set(const char **nb_addresses, size_t n_addresses, +update_sb_addr_set(struct sorted_addresses *nb_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)); + size_t nb_index, sb_index; + + const char **nb_arr = nb_addresses->arr; + char **sb_arr = sb_as->addresses; + size_t nb_n = nb_addresses->n; + size_t sb_n = sb_as->n_addresses; + + for (nb_index = sb_index = 0; nb_index < nb_n && sb_index < sb_n; ) { + int cmp = strcmp(nb_arr[nb_index], sb_arr[sb_index]); + if (cmp < 0) { + sbrec_address_set_update_addresses_addvalue(sb_as, + nb_arr[nb_index]); + nb_index++; + } else if (cmp > 0) { + sbrec_address_set_update_addresses_delvalue(sb_as, + sb_arr[sb_index]); + sb_index++; + } else { + nb_index++; + sb_index++; } } - 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)); - } + for (; nb_index < nb_n; nb_index++) { + sbrec_address_set_update_addresses_addvalue(sb_as, nb_arr[nb_index]); } - 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); + for (; sb_index < sb_n; sb_index++) { + sbrec_address_set_update_addresses_delvalue(sb_as, sb_arr[sb_index]); + } } static void @@ -402,3 +414,32 @@ sb_address_set_lookup_by_name(struct ovsdb_idl_index *sbrec_addr_set_by_name, return retval; } + +static struct sorted_addresses +sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as) +{ + /* The DB is already sorted. */ + return (struct sorted_addresses) { + .arr = (const char **) nb_as->addresses, + .n = nb_as->n_addresses, + }; +} + +static struct sorted_addresses +sorted_addresses_from_svec(struct svec *addresses) +{ + svec_sort(addresses); + return (struct sorted_addresses) { + .arr = (const char **) addresses->names, + .n = addresses->n, + }; +} + +static struct sorted_addresses +sorted_addresses_from_sset(struct sset* addresses) +{ + return (struct sorted_addresses) { + .arr = sset_sort(addresses), + .n = sset_count(addresses), + }; +} diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 6736429ae..64bb9495a 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -8835,8 +8835,9 @@ 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 as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats -check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.3 -wait_column '1.1.1.1 1.1.1.2 1.1.1.3' Address_Set addresses name=foo +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.3 -- \ + add address_set $foo_as_uuid addresses 1.1.2.1/4 +wait_column '1.1.1.1 1.1.1.2 1.1.1.3 1.1.2.1/4' Address_Set addresses name=foo # There should be no recompute of the sync_to_sb_addr_set engine node . AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_to_sb_addr_set recompute], [0], [0 @@ -8846,8 +8847,9 @@ AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \ grep -c mutate], [0], [1 ]) -check ovn-nbctl add address_set $foo_as_uuid addresses \ -1.1.1.4 -- remove address_set $foo_as_uuid addresses 1.1.1.1 +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.4 -- \ + remove address_set $foo_as_uuid addresses 1.1.1.1 -- \ + remove address_set $foo_as_uuid addresses 1.1.2.1/4 wait_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set addresses name=foo # There should be no recompute of the sync_to_sb_addr_set engine node .
The incremental processing is broken for addresses that have mask which could "erase" portion of the address itself e.g. 10.16.0.47/4, after applying the mask with token parser the address becomes 0.0.0.0/4, which is fine for logical flows. However, for the deletion/addition to database we need the original string representation which cannot be built from the parsed token. Use strcmp over already sorted lists which should give us the needed diff. The time complexity didn't change, but the performance was slightly improved, see the numbers below. The setup was created by the scale script from Han [0]. Numbers with expr: Time spent on processing nb_cfg 1: ovn-northd delay before processing: 21ms ovn-northd completion: 544ms ovn-controller(s) completion: 544ms Time spent on processing nb_cfg 2: ovn-northd delay before processing: 17ms ovn-northd completion: 537ms ovn-controller(s) completion: 535ms Time spent on processing nb_cfg 3: ovn-northd delay before processing: 20ms ovn-northd completion: 552ms ovn-controller(s) completion: 550ms Time spent on processing nb_cfg 4: ovn-northd delay before processing: 16ms ovn-northd completion: 529ms ovn-controller(s) completion: 528ms Numbers with the strcmp: Time spent on processing nb_cfg 1: ovn-northd delay before processing: 24ms ovn-northd completion: 521ms ovn-controller(s) completion: 521ms Time spent on processing nb_cfg 2: ovn-northd delay before processing: 17ms ovn-northd completion: 500ms ovn-controller(s) completion: 500ms Time spent on processing nb_cfg 3: ovn-northd delay before processing: 17ms ovn-northd completion: 496ms ovn-controller(s) completion: 497ms Time spent on processing nb_cfg 4: ovn-northd delay before processing: 18ms ovn-northd completion: 502ms ovn-controller(s) completion: 500ms [0] https://github.com/hzhou8/ovn-test-script/blob/e1149318201309db6d96ae8d5a33fcfbbe1872a3/test_ovn_controller_scale.sh Fixes: 0d5e0a6ced49 ("northd: Add I-P for syncing SB address sets.") Reported-at: https://bugzilla.redhat.com/2196885 Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2023-May/052426.html Reported-By: 张祖建 <zhangzujian.7@gmail.com> Signed-off-by: Ales Musil <amusil@redhat.com> --- v2: Address comments from Ilya: - Use "custom" algorithm to prevent a ton of allocations with svec. - Add original reporter. v3: Rebase on top of current main. Address comment from Han: - Add test case for the problematic address. --- northd/en-sync-sb.c | 183 +++++++++++++++++++++++++++----------------- tests/ovn-northd.at | 10 ++- 2 files changed, 118 insertions(+), 75 deletions(-)