'
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.
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.
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
