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 ?
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
