On Mon, Nov 14, 2022 at 3:23 PM Mark Michelson <[email protected]> wrote:
>
> Hi Numan, I have just one minor suggestion below.
>
> On 11/14/22 11:48, [email protected] wrote:
> > From: Numan Siddique <[email protected]>
> >
> > Updates to NB address sets and NB port groups are handled
> > incrementally for syncing the SB address sets. This patch
> > doesn't support syncing the SB Address sets for the router
> > load balancer vips incrementally, instead a full recompute is
> > triggered for any changes to NB load balancers, NB load balancer
> > groups and NB logical routers.
> >
> > Signed-off-by: Numan Siddique <[email protected]>
> > ---
> > northd/en-sb-sync.c | 202 ++++++++++++++++++++++++++++++++++++---
> > northd/en-sb-sync.h | 6 ++
> > northd/inc-proc-northd.c | 18 +++-
> > tests/ovn-northd.at | 52 ++++++++++
> > 4 files changed, 260 insertions(+), 18 deletions(-)
> >
> > diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c
> > index c3ba315df..8a17998ec 100644
> > --- a/northd/en-sb-sync.c
> > +++ b/northd/en-sb-sync.c
> > @@ -22,6 +22,7 @@
> > #include "openvswitch/util.h"
> >
> > #include "en-sb-sync.h"
> > +#include "include/ovn/expr.h"
> > #include "lib/inc-proc-eng.h"
> > #include "lib/lb.h"
> > #include "lib/ovn-nb-idl.h"
> > @@ -41,6 +42,13 @@ static void sync_address_sets(const struct
> > nbrec_address_set_table *,
> > const struct sbrec_address_set_table *,
> > struct ovsdb_idl_txn *ovnsb_txn,
> > struct hmap *datapaths);
> > +static const struct sbrec_address_set *sb_address_set_lookup_by_name(
> > + struct ovsdb_idl_index *, const char *name);
> > +static void update_sb_addr_set(const char **nb_addresses, size_t
> > n_addresses,
> > + const struct sbrec_address_set *);
> > +static void build_port_group_address_set(const struct nbrec_port_group *,
> > + struct svec *ipv4_addrs,
> > + struct svec *ipv6_addrs);
> >
> > void *
> > en_sb_sync_init(struct engine_node *node OVS_UNUSED,
> > @@ -94,6 +102,98 @@ en_address_set_sync_cleanup(void *data OVS_UNUSED)
> >
> > }
> >
> > +bool
> > +address_set_sync_nb_address_set_handler(struct engine_node *node
> > OVS_UNUSED,
> > + void *data OVS_UNUSED)
> > +{
> > + const struct nbrec_address_set_table *nb_address_set_table =
> > + EN_OVSDB_GET(engine_get_input("NB_address_set", node));
> > +
> > + /* Return false if an address set is created or deleted.
> > + * Handle I-P for only updated address sets. */
> > + const struct nbrec_address_set *nb_addr_set;
> > + NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (nb_addr_set,
> > + nb_address_set_table) {
> > + if (nbrec_address_set_is_new(nb_addr_set) ||
> > + nbrec_address_set_is_deleted(nb_addr_set)) {
> > + return false;
> > + }
> > + }
> > +
> > + struct ovsdb_idl_index *sbrec_address_set_by_name =
> > + engine_ovsdb_node_get_index(
> > + engine_get_input("SB_address_set", node),
> > + "sbrec_address_set_by_name");
> > +
> > + NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (nb_addr_set,
> > + nb_address_set_table) {
> > + const struct sbrec_address_set *sb_addr_set =
> > + sb_address_set_lookup_by_name(sbrec_address_set_by_name,
> > + nb_addr_set->name);
> > + if (!sb_addr_set) {
> > + return false;
> > + }
> > + update_sb_addr_set((const char **) nb_addr_set->addresses,
> > + nb_addr_set->n_addresses, sb_addr_set);
> > + }
> > +
> > + return true;
> > +}
> > +
> > +bool
> > +address_set_sync_nb_port_group_handler(struct engine_node *node OVS_UNUSED,
> > + void *data OVS_UNUSED)
> > +{
> > + const struct nbrec_port_group *nb_pg;
> > + const struct nbrec_port_group_table *nb_port_group_table =
> > + EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, nb_port_group_table) {
> > + if (nbrec_port_group_is_new(nb_pg) ||
> > + nbrec_port_group_is_deleted(nb_pg)) {
> > + return false;
> > + }
> > + }
> > +
> > + struct ovsdb_idl_index *sbrec_address_set_by_name =
> > + engine_ovsdb_node_get_index(
> > + engine_get_input("SB_address_set", node),
> > + "sbrec_address_set_by_name");
> > + NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, nb_port_group_table) {
> > + char *ipv4_addrs_name = xasprintf("%s_ip4", nb_pg->name);
> > + const struct sbrec_address_set *sb_addr_set_v4 =
> > + sb_address_set_lookup_by_name(sbrec_address_set_by_name,
> > + ipv4_addrs_name);
> > + if (!sb_addr_set_v4) {
> > + free(ipv4_addrs_name);
> > + return false;
> > + }
> > + char *ipv6_addrs_name = xasprintf("%s_ip6", nb_pg->name);
> > + const struct sbrec_address_set *sb_addr_set_v6 =
> > + sb_address_set_lookup_by_name(sbrec_address_set_by_name,
> > + ipv6_addrs_name);
> > + if (!sb_addr_set_v6) {
> > + free(ipv4_addrs_name);
> > + free(ipv6_addrs_name);
> > + return false;
> > + }
> > +
> > + struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER;
> > + struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER;
> > + build_port_group_address_set(nb_pg, &ipv4_addrs, &ipv6_addrs);
> > + update_sb_addr_set((const char **) ipv4_addrs.names, ipv4_addrs.n,
> > + sb_addr_set_v4);
> > + update_sb_addr_set((const char **) ipv6_addrs.names, ipv6_addrs.n,
> > + sb_addr_set_v6);
> > +
> > + free(ipv4_addrs_name);
> > + free(ipv6_addrs_name);
> > + svec_destroy(&ipv4_addrs);
> > + svec_destroy(&ipv6_addrs);
> > + }
> > +
> > + return true;
> > +}
> > +
> > /* static functions. */
> > static void
> > sync_address_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name,
> > @@ -106,10 +206,11 @@ sync_address_set(struct ovsdb_idl_txn *ovnsb_txn,
> > const char *name,
> > if (!sb_address_set) {
> > sb_address_set = sbrec_address_set_insert(ovnsb_txn);
> > sbrec_address_set_set_name(sb_address_set, name);
> > + sbrec_address_set_set_addresses(sb_address_set,
> > + addrs, n_addrs);
> > + } else {
> > + update_sb_addr_set(addrs, n_addrs, sb_address_set);
> > }
> > -
> > - sbrec_address_set_set_addresses(sb_address_set,
> > - addrs, n_addrs);
> > }
> >
> > /* OVN_Southbound Address_Set table contains same records as in north
> > @@ -148,18 +249,7 @@ sync_address_sets(
> > nb_port_group_table) {
> > struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER;
> > struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER;
> > - for (size_t i = 0; i < nb_port_group->n_ports; i++) {
> > - for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses;
> > j++) {
> > - const char *addrs = nb_port_group->ports[i]->addresses[j];
> > - if (!is_dynamic_lsp_address(addrs)) {
> > - split_addresses(addrs, &ipv4_addrs, &ipv6_addrs);
> > - }
> > - }
> > - if (nb_port_group->ports[i]->dynamic_addresses) {
> > - split_addresses(nb_port_group->ports[i]->dynamic_addresses,
> > - &ipv4_addrs, &ipv6_addrs);
> > - }
> > - }
> > + build_port_group_address_set(nb_port_group, &ipv4_addrs,
> > &ipv6_addrs);
> > char *ipv4_addrs_name = xasprintf("%s_ip4", nb_port_group->name);
> > char *ipv6_addrs_name = xasprintf("%s_ip6", nb_port_group->name);
> > sync_address_set(ovnsb_txn, ipv4_addrs_name,
> > @@ -228,3 +318,85 @@ sync_address_sets(
> > }
> > shash_destroy(&sb_address_sets);
> > }
> > +
> > +static void
> > +update_sb_addr_set(const char **nb_addresses, size_t n_addresses,
> > + const struct sbrec_address_set *sb_as)
> > +{
> > + struct expr_constant_set *cs_nb_as =
> > + expr_constant_set_create_integers(
> > + (const char *const *) nb_addresses, n_addresses);
> > + struct expr_constant_set *cs_sb_as =
> > + expr_constant_set_create_integers(
> > + (const char *const *) sb_as->addresses, sb_as->n_addresses);
> > +
> > + struct expr_constant_set *addr_added = NULL;
> > + struct expr_constant_set *addr_deleted = NULL;
> > + expr_constant_set_integers_diff(cs_sb_as, cs_nb_as, &addr_added,
> > + &addr_deleted);
> > +
> > + if (addr_added && addr_added->n_values) {
> > + for (size_t i = 0; i < addr_added->n_values; i++) {
> > + struct ds ds = DS_EMPTY_INITIALIZER;
> > + expr_constant_format(&addr_added->values[i], EXPR_C_INTEGER,
> > &ds);
> > + sbrec_address_set_update_addresses_addvalue(sb_as,
> > ds_cstr(&ds));
> > + ds_destroy(&ds);
>
> Nit: Instead of creating and destroying a dynamic string in each loop
> iteration, how about creating a single dynamic string and clearing it at
> the beginning of each iteration? Then you can destroy the dynamic string
> at the end of the function. I don't think it will have a huge impact on
> performance, but it certainly should reduce the number of allocations.
Thanks for the review.
Sounds good to me. Shall I respin another version or wait for more comments ?
Thanks
Numan
>
> > + }
> > + }
> > +
> > + if (addr_deleted && addr_deleted->n_values) {
> > + for (size_t i = 0; i < addr_deleted->n_values; i++) {
> > + struct ds ds = DS_EMPTY_INITIALIZER;
> > + expr_constant_format(&addr_deleted->values[i],
> > + EXPR_C_INTEGER, &ds);
> > + sbrec_address_set_update_addresses_delvalue(sb_as,
> > ds_cstr(&ds));
> > + ds_destroy(&ds);
> > + }
> > + }
> > +
> > + expr_constant_set_destroy(cs_nb_as);
> > + free(cs_nb_as);
> > + expr_constant_set_destroy(cs_sb_as);
> > + free(cs_sb_as);
> > + expr_constant_set_destroy(addr_added);
> > + free(addr_added);
> > + expr_constant_set_destroy(addr_deleted);
> > + free(addr_deleted);
> > +}
> > +
> > +static void
> > +build_port_group_address_set(const struct nbrec_port_group *nb_port_group,
> > + struct svec *ipv4_addrs,
> > + struct svec *ipv6_addrs)
> > +{
> > + for (size_t i = 0; i < nb_port_group->n_ports; i++) {
> > + for (size_t j = 0; j < nb_port_group->ports[i]->n_addresses; j++) {
> > + const char *addrs = nb_port_group->ports[i]->addresses[j];
> > + if (!is_dynamic_lsp_address(addrs)) {
> > + split_addresses(addrs, ipv4_addrs, ipv6_addrs);
> > + }
> > + }
> > + if (nb_port_group->ports[i]->dynamic_addresses) {
> > + split_addresses(nb_port_group->ports[i]->dynamic_addresses,
> > + ipv4_addrs, ipv6_addrs);
> > + }
> > + }
> > +}
> > +
> > +/* Finds and returns the address set with the given 'name', or NULL if no
> > such
> > + * address set exists. */
> > +static const struct sbrec_address_set *
> > +sb_address_set_lookup_by_name(struct ovsdb_idl_index
> > *sbrec_addr_set_by_name,
> > + const char *name)
> > +{
> > + struct sbrec_address_set *target = sbrec_address_set_index_init_row(
> > + sbrec_addr_set_by_name);
> > + sbrec_address_set_index_set_name(target, name);
> > +
> > + struct sbrec_address_set *retval = sbrec_address_set_index_find(
> > + sbrec_addr_set_by_name, target);
> > +
> > + sbrec_address_set_index_destroy_row(target);
> > +
> > + return retval;
> > +}
> > diff --git a/northd/en-sb-sync.h b/northd/en-sb-sync.h
> > index f99d6a9fc..a63453fe5 100644
> > --- a/northd/en-sb-sync.h
> > +++ b/northd/en-sb-sync.h
> > @@ -3,12 +3,18 @@
> >
> > #include "lib/inc-proc-eng.h"
> >
> > +/* en_sb_sync engine node functions. */
> > void *en_sb_sync_init(struct engine_node *, struct engine_arg *);
> > void en_sb_sync_run(struct engine_node *, void *data);
> > void en_sb_sync_cleanup(void *data);
> >
> > +/* en_address_set_sync engine node functions. */
> > void *en_address_set_sync_init(struct engine_node *, struct engine_arg *);
> > void en_address_set_sync_run(struct engine_node *, void *data);
> > void en_address_set_sync_cleanup(void *data);
> > +bool address_set_sync_nb_address_set_handler(struct engine_node *,
> > + void *data);
> > +bool address_set_sync_nb_port_group_handler(struct engine_node *,
> > + void *data);
> >
> > #endif
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index b48f53f17..e2c25046a 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -238,8 +238,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > * the NB database tables.
> > * Right now this engine only syncs the SB Address_Set table.
> > */
> > - engine_add_input(&en_address_set_sync, &en_nb_address_set, NULL);
> > - engine_add_input(&en_address_set_sync, &en_nb_port_group, NULL);
> > + engine_add_input(&en_address_set_sync, &en_nb_address_set,
> > + address_set_sync_nb_address_set_handler);
> > + engine_add_input(&en_address_set_sync, &en_nb_port_group,
> > + address_set_sync_nb_port_group_handler);
> > engine_add_input(&en_address_set_sync, &en_nb_load_balancer, NULL);
> > engine_add_input(&en_address_set_sync, &en_nb_load_balancer_group,
> > NULL);
> > engine_add_input(&en_address_set_sync, &en_nb_logical_router, NULL);
> > @@ -248,8 +250,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >
> > /* We need the en_northd generated data as input to
> > en_address_set_sync
> > * node to access the data generated by it (eg. struct ovn_datapath).
> > + * The handler is noop since en_northd always falls back to full
> > recompute
> > + * (since it has no input handlers) and it doesn't yet indicate what
> > + * changed. It doesn't make sense to add NULL handler for this input,
> > + * otherwise 'en_address_set_sync' will always fall back to full
> > recompute.
> > */
> > - engine_add_input(&en_address_set_sync, &en_northd, NULL);
> > + engine_add_input(&en_address_set_sync, &en_northd,
> > engine_noop_handler);
> >
> > engine_add_input(&en_sb_sync, &en_address_set_sync, NULL);
> > engine_add_input(&en_northd_output, &en_sb_sync,
> > @@ -300,6 +306,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_ovsdb_node_add_index(&en_sb_mac_binding,
> > "sbrec_mac_binding_by_datapath",
> > sbrec_mac_binding_by_datapath);
> > +
> > + struct ovsdb_idl_index *sbrec_address_set_by_name
> > + = ovsdb_idl_index_create1(sb->idl, &sbrec_address_set_col_name);
> > + engine_ovsdb_node_add_index(&en_sb_address_set,
> > + "sbrec_address_set_by_name",
> > + sbrec_address_set_by_name);
> > }
> >
> > void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 4f399eccb..f924dcfef 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -7929,3 +7929,55 @@ AT_CHECK([grep "lr_in_arp_resolve" R1flows | grep
> > priority=90 | sort], [0], [dnl
> >
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Address set incremental processing])
> > +ovn_start
> > +
> > +foo_as_uuid=$(ovn-nbctl create address_set name=foo
> > addresses=\"1.1.1.1\",\"1.1.1.2\")
> > +ovn-nbctl --wait=sb sync
> > +
> > +check_column '1.1.1.1 1.1.1.2' Address_Set addresses name=foo
> > +
> > +rm -f northd/ovn-northd.log
> > +check as northd ovn-appctl -t NORTHD_TYPE vlog/reopen
> > +check as northd ovn-appctl -t NORTHD_TYPE vlog/set jsonrpc:dbg
> > +
> > +check ovn-nbctl --wait=sb add address_set $foo_as_uuid addresses 1.1.1.3
> > +check_column '1.1.1.1 1.1.1.2 1.1.1.3' Address_Set addresses name=foo
> > +
> > +AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \
> > +grep -c mutate], [0], [1
> > +])
> > +
> > +check ovn-nbctl --wait=sb add address_set $foo_as_uuid addresses \
> > +1.1.1.4 -- remove address_set $foo_as_uuid addresses 1.1.1.1
> > +check_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set addresses name=foo
> > +
> > +AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \
> > +grep -c mutate], [0], [2
> > +])
> > +
> > +# Pause ovn-northd and add/remove few addresses. when it is resumed
> > +# it should use mutate for updating the address sets.
> > +check as northd ovn-appctl -t NORTHD_TYPE pause
> > +check as northd-backup ovn-appctl -t NORTHD_TYPE pause
> > +
> > +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.5
> > +check ovn-nbctl add address_set $foo_as_uuid addresses 1.1.1.6
> > +check ovn-nbctl remove address_set $foo_as_uuid addresses 1.1.1.2
> > +
> > +check_column '1.1.1.2 1.1.1.3 1.1.1.4' Address_Set addresses name=foo
> > +
> > +# Resume northd now
> > +check as northd ovn-appctl -t NORTHD_TYPE resume
> > +check ovn-nbctl --wait=sb sync
> > +
> > +check_column '1.1.1.3 1.1.1.4 1.1.1.5 1.1.1.6' Address_Set addresses
> > name=foo
> > +
> > +AT_CHECK([grep transact northd/ovn-northd.log | grep Address_Set | \
> > +grep -c mutate], [0], [3
> > +])
> > +
> > +AT_CLEANUP
> > +])
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
